Review Request 118966: Avoid opening unused tabs which are closed again after startup has finished (when directory/file urls are given)

[Frank Reininghaus]

> On July 6, 2014, 9:04 p.m., Frank Reininghaus wrote:
> > Thanks for the patch and sorry for not responding earlier. This is a really good idea, please go ahead and push it.
> 
> Frank Reininghaus wrote:
>     I get a crash with this patch if "Split View" is enabled in the settings and I run "dolphin ." from Konsole.
>     
>     [KCrash Handler]
>     #6  0x00007f9e833e969e in DolphinViewContainer::view (this=0x0) at /home/kde-devel/kde/src/kde/applications/kde-baseapps/dolphin/src/dolphinviewcontainer.cpp:256
>     #7  0x00007f9e833d984c in DolphinMainWindow::showEvent (this=0x2577e60, event=0x7fffbac60520) at /home/kde-devel/kde/src/kde/applications/kde-baseapps/dolphin/src/dolphinmainwindow.cpp:501
>     #8  0x00007f9e7f32d899 in QWidget::event (this=0x2577e60, event=0x7fffbac60520) at kernel/qwidget.cpp:8607
>     #9  0x00007f9e7f8052ea in QMainWindow::event (this=0x2577e60, event=0x7fffbac60520) at widgets/qmainwindow.cpp:1478
>     #10 0x00007f9e8040b738 in KMainWindow::event (this=0x2577e60, ev=0x7fffbac60520) at /home/kde-devel/kde/src/kde/kdelibs/kdeui/widgets/kmainwindow.cpp:1084
>     #11 0x00007f9e804588a3 in KXmlGuiWindow::event (this=0x2577e60, ev=0x7fffbac60520) at /home/kde-devel/kde/src/kde/kdelibs/kdeui/xmlgui/kxmlguiwindow.cpp:126
>     #12 0x00007f9e7f2c762e in QApplicationPrivate::notify_helper (this=0x2452fd0, receiver=0x2577e60, e=0x7fffbac60520) at kernel/qapplication.cpp:4565
>     #13 0x00007f9e7f2c72e1 in QApplication::notify (this=0x7fffbac607c0, receiver=0x2577e60, e=0x7fffbac60520) at kernel/qapplication.cpp:4530
>     #14 0x00007f9e8031cc1f in KApplication::notify (this=0x7fffbac607c0, receiver=0x2577e60, event=0x7fffbac60520) at /home/kde-devel/kde/src/kde/kdelibs/kdeui/kernel/kapplication.cpp:311
>     #15 0x00007f9e7e2778bf in QCoreApplication::notifyInternal (this=0x7fffbac607c0, receiver=0x2577e60, event=0x7fffbac60520) at kernel/qcoreapplication.cpp:953
>     #16 0x00007f9e8218aecf in QCoreApplication::sendEvent (receiver=0x2577e60, event=0x7fffbac60520) at ../../include/QtCore/../../src/corelib/kernel/qcoreapplication.h:231
>     #17 0x00007f9e7f32b04c in QWidgetPrivate::show_helper (this=0x25ba290) at kernel/qwidget.cpp:7569
>     #18 0x00007f9e7f32b98d in QWidget::setVisible (this=0x2577e60, visible=true) at kernel/qwidget.cpp:7791
>     #19 0x00007f9e833d5a40 in QWidget::show (this=0x2577e60) at /home/kde-devel/qt/include/QtGui/../../src/gui/kernel/qwidget.h:497
>     #20 0x00007f9e833d5504 in DolphinApplication::DolphinApplication (this=0x7fffbac607c0) at /home/kde-devel/kde/src/kde/applications/kde-baseapps/dolphin/src/dolphinapplication.cpp:84
>     #21 0x00007f9e833f4572 in kdemain (argc=2, argv=0x7fffbac60d68) at /home/kde-devel/kde/src/kde/applications/kde-baseapps/dolphin/src/main.cpp:89
>     #22 0x0000000000400aa2 in main (argc=2, argv=0x7fffbac60d68) at /home/kde-devel/kde/build/kde/applications/kde-baseapps/dolphin/src/dolphin_dummy.cpp:3

I think the problem is that m_activeViewContainer is 0 if all tabs have been opened inside openDirectories(const QList<KUrl>& dirs).

The good thing about the current code (which always opens an activated tab in the DolphinMainWindow constructor) is that it is always guaranteed that there is at least one tab and one active view container. This property gets lost with this patch, but I still agree that the idea of not opening tabs which would later be closed anyway does make sense.

A possible solution would be to 

1. In DolphinMainWindow::showEvent(), check if there is at least one tab, and open one for the home URL if that is not the case (to prevent that the code gets fragile if the DolphinApplication code gets changed at some point in the future). One could remove the call "m_mainWindow->openNewActivatedTab(homeUrl);" in the new 'else' branch in DolphinApplication then.

2. After that, check in showEvent() if there is an active view container, and just take the first view container in the first tab if that is not the case.

That idea might conflict with your DolphinTabWidget patch though, and might need to be changed.


- Frank


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118966/#review61750
-----------------------------------------------------------


On June 26, 2014, 10:09 p.m., Emmanuel Pescosta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118966/
> -----------------------------------------------------------
> 
> (Updated June 26, 2014, 10:09 p.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Repository: kde-baseapps
> 
> 
> Description
> -------
> 
> Instead of always opening a new tab with the home url and closing it again when directory/file urls are passed on, 
> we now use the given directories/files directly to open new tabs on startup.
> 
> Makes the code easier and we can reuse openDirectories/openFiles in future (if needed).
> 
> 
> Diffs
> -----
> 
>   dolphin/src/dolphinapplication.cpp 8e83a85 
>   dolphin/src/dolphinmainwindow.cpp c60951d 
> 
> Diff: https://git.reviewboard.kde.org/r/118966/diff/
> 
> 
> Testing
> -------
> 
> Works fine.
> 
> 
> Thanks,
> 
> Emmanuel Pescosta
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20140706/b6d96c01/attachment.htm>