Review Request: Focus goes to location bar when opening link in new tab in foreground

[Dawit Alemayehu]

> On Aug. 17, 2012, 8:29 p.m., David Faure wrote:
> > Not a strong objection, but KonqViewManager::doSetActivePart is supposed to do this already, so I'm surprised it doesn't work?
> > 
> > If that method isn't called in your case, then OK.

Well I finally figured out why this was happening! Believe it or not it is due to a fix for another bug in konqFrame::activateChild(). Below is the sequence of actions that result in the aforementioned bug report and potentially also bug# 304865.

When KonqMainWindow's slotCreateNewWindow function is called, it calls KonqViewManager::showTab when it is configured to put the newly created tab on top. This happens in either slotCreateNewWindow (line #1221) itself or in the subsequent call to openUrl (line# 596).

KonqViewManager::showTab calls KonqFrameTabs::setCurrentIndex to change the current active tab to the new tab. KonqFrameTabs::setCurrentIndex is actually QTabWidget::setCurrentIndex which emits the signal QTabWidget::currentChanged. 

KonqFrameTabs connects the QTabWidget::currentChanged signal to its slot KonqFrameTabs::slotCurrentChanged. KonqFrameTabs::slotCurrentChanged then ends up calling KonqFrameBase::activateChild (which is KonqFrame::activateChild).

And in KonqFrameBase::activateChild we have the offending code @ line # 226-228:

        if (m_pView->url().isEmpty() || m_pView->url() == "about:blank") {
            m_pView->mainWindow()->focusLocationBar(); // #84867 usability improvement
        }

>From your commit log, the above fix seem to have been applied elsewhere but was moved here by you so that it won't cause another regression. Commit revisiion de3e94e9 says:

Re-do the usability fix #84867 in a way that doesn't introduce #208821:
focus the location bar when actively (as a user) switching to an empty tab,
but not when loading a tab which will soon get content.
Fixed for: 4.5
BUG: 208821


So in short, a fix for a regression (208821) caused by a fix for a bug (84867) is caused yet another regression (304933). Not sure what the appropriate fix here just yet. We actually have to find a way to fix 84867 without causing the other two regressions or any new ones for that matter.


- Dawit


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105984/#review17622
-----------------------------------------------------------


On Aug. 11, 2012, 3:58 p.m., Dawit Alemayehu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105984/
> -----------------------------------------------------------
> 
> (Updated Aug. 11, 2012, 3:58 p.m.)
> 
> 
> Review request for KDE Base Apps and David Faure.
> 
> 
> Description
> -------
> 
> The attached patch address the bug reported in #304933. Right now if Konqueror is configured to open new tabs in the foreground, i.e. the "Open tabs in the background" option is unchecked, then the keyboard focus is put on the location bar instead of the view.
> 
> 
> This addresses bug 304933.
>     http://bugs.kde.org/show_bug.cgi?id=304933
> 
> 
> Diffs
> -----
> 
>   konqueror/src/konqmainwindow.cpp 6faba58 
> 
> Diff: http://git.reviewboard.kde.org/r/105984/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Dawit Alemayehu
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20120818/d61a3b1c/attachment.htm>