Discussion:
D16648: Open externally called files/directories in new tabs
Alexander Saoutkin
2018-11-03 18:43:58 UTC
Permalink
feverfew created this revision.
Herald added a project: Dolphin.
Herald added a subscriber: kfm-devel.
feverfew requested review of this revision.

REVISION SUMMARY
BUG: 183429

Added option allowing user to choose if they would want to open
externally called files/directories in new tabs. This option is enabled by default.

When this option is selected externally called files/directories are opened in a a new tab of an instance of Dolphin that already exists. If there is no instance then the files/directories are opened in a new window. The newly opened file/directory has its tab activated, and consequently, the window is also activated.

When the user clicks "Open In New Window" or "Detach Tab", the files/directories are opened in a new window, regardless of the setting chosen.

TEST PLAN
[Manual]
Before testing, set the default file manager as the newly built Dolphin executable.
Please note that when testing the "Open In New Window"/"Detach Tab" feature, an old instance of dolphin will be opened (in particular, the executable in your /usr/ directory), to properly test please change the source code to the new Dolphin executable.

Furthermore, running two different versions of Dolphin (in particular, where one does not have this patch included) can result in bugs appearing when the setting is enabled, in particular, new tabs not opening as old instances will not recognise the DBus commands send to it. However, I see no reason why a user will have two different versions of Dolphin (apart from people like us :D).

With setting off:
Application should behave as before.
With setting on:
Open directories with the help of auxillary programs (i.e. a browser). The files/directories should appear in a new window if an instance does not exist. If one already does, then a new tab should be opened and activated in that instance and the window activated.
When a user chooses to "Open In New Window"/"Detach Tab" then the files/directories should be opened in a new window.

REPOSITORY
R318 Dolphin

BRANCH
DBusTabInstance

REVISION DETAIL
https://phabricator.kde.org/D16648

AFFECTED FILES
CMakeLists.txt
src/CMakeLists.txt
src/dbusinterface.cpp
src/dolphinmainwindow.cpp
src/dolphinmainwindow.h
src/dolphintabwidget.cpp
src/global.cpp
src/global.h
src/main.cpp
src/settings/dolphin_generalsettings.kcfg
src/settings/startup/startupsettingspage.cpp
src/settings/startup/startupsettingspage.h
src/tests/dolphinmainwindowtest.cpp

To: feverfew
Cc: kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp
Alexander Saoutkin
2018-11-03 18:46:26 UTC
Permalink
feverfew updated this revision to Diff 44798.
feverfew added a comment.


- Removed unnecessary string

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16648?vs=44796&id=44798

BRANCH
DBusTabInstance

REVISION DETAIL
https://phabricator.kde.org/D16648

AFFECTED FILES
CMakeLists.txt
src/CMakeLists.txt
src/dbusinterface.cpp
src/dolphinmainwindow.cpp
src/dolphinmainwindow.h
src/dolphintabwidget.cpp
src/global.cpp
src/global.h
src/main.cpp
src/settings/dolphin_generalsettings.kcfg
src/settings/startup/startupsettingspage.cpp
src/settings/startup/startupsettingspage.h
src/tests/dolphinmainwindowtest.cpp

To: feverfew
Cc: kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp
Anthony Fieroni
2018-11-03 18:53:47 UTC
Permalink
anthonyfieroni added a comment.


--new-window will be more readable, about me. Add braces on one line conditions, tryRise should be raise ('try' does not make sense).

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D16648

To: feverfew
Cc: anthonyfieroni, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp
Alexander Saoutkin
2018-11-03 19:55:39 UTC
Permalink
feverfew updated this revision to Diff 44801.
feverfew added a comment.


- Make new window option clearer, remove unnecessary string and whitespace

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16648?vs=44798&id=44801

BRANCH
DBusTabInstance

REVISION DETAIL
https://phabricator.kde.org/D16648

AFFECTED FILES
CMakeLists.txt
src/CMakeLists.txt
src/dbusinterface.cpp
src/dolphinmainwindow.cpp
src/dolphinmainwindow.h
src/dolphintabwidget.cpp
src/global.cpp
src/global.h
src/main.cpp
src/settings/dolphin_generalsettings.kcfg
src/settings/startup/startupsettingspage.cpp
src/settings/startup/startupsettingspage.h
src/tests/dolphinmainwindowtest.cpp

To: feverfew
Cc: anthonyfieroni, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp
Nathaniel Graham
2018-11-04 05:18:58 UTC
Permalink
ngraham added a reviewer: Dolphin.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D16648

To: feverfew, #dolphin
Cc: anthonyfieroni, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov
Elvis Angelaccio
2018-11-04 17:58:49 UTC
Permalink
elvisangelaccio added a comment.


Thanks for the patch. Before I look at the code: why a new option for this feature?

We should add new options only if there is a really good reason, because the more options we have the more complex the codebase becomes.

Also, looking at the bugzilla ticket it seems that this feature was already in konqueror and it was not ported to dolphin because it didn't have tabs at the time. That's one more reason to not make an option out of this.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D16648

To: feverfew, #dolphin
Cc: elvisangelaccio, anthonyfieroni, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov
Alexander Saoutkin
2018-11-04 20:41:41 UTC
Permalink
feverfew added a comment.
Post by Anthony Fieroni
--new-window will be more readable, about me. Add braces on one line conditions, tryRise should be raise ('try' does not make sense).
Ok will add braces once we get a review of whether the patch should remain as an option.

The 'try' is there for two reasons. Firstly, raise() is already a function ( that is inherited) in DolphinMainWindow. Secondly, Qt does not allow you to raise a minimised a window above all others without you explicitly clicking on it in the task manager, or some other way. Although, KWindowSystem seems to have hacked something together to allow us to do so, as it makes sense in this case. As I was making this patch and reading around, the window may not always be raised, depending on the window manager.
Post by Anthony Fieroni
Thanks for the patch. Before I look at the code: why a new option for this feature?
We should add new options only if there is a really good reason, because the more options we have the more complex the codebase becomes.
Also, looking at the bugzilla ticket it seems that this feature was already in konqueror and it was not ported to dolphin because it didn't have tabs at the time. That's one more reason to not make an option out of this.
Because it will be a big (but positive) change for some users. Of course, there is a reason why the setting is set as default, and only the change in how the program works is why I added it as an option. I am happy to let people discuss whether it should appear as an option. I personally, would actually prefer it to not be an option.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D16648

To: feverfew, #dolphin
Cc: elvisangelaccio, anthonyfieroni, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov
Elvis Angelaccio
2018-11-18 10:21:02 UTC
Permalink
elvisangelaccio added a comment.


I vote to not make it an option. There is always time to reconsider if enough people complain about the new behavior :)

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D16648

To: feverfew, #dolphin
Cc: elvisangelaccio, anthonyfieroni, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov
Nathaniel Graham
2018-11-19 03:32:44 UTC
Permalink
ngraham added a comment.


I'm also okay with not making it an option for now, pending real-world user feedback. :)

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D16648

To: feverfew, #dolphin
Cc: ngraham, elvisangelaccio, anthonyfieroni, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov
Alexander Saoutkin
2018-11-23 18:41:08 UTC
Permalink
feverfew updated this revision to Diff 46087.
feverfew added a comment.


- Make new window option clearer, remove unnecessary string and whitespace

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16648?vs=44801&id=46087

BRANCH
DBusTabInstance

REVISION DETAIL
https://phabricator.kde.org/D16648

AFFECTED FILES
CMakeLists.txt
src/CMakeLists.txt
src/dbusinterface.cpp
src/dolphinmainwindow.cpp
src/dolphinmainwindow.h
src/dolphintabwidget.cpp
src/global.cpp
src/global.h
src/main.cpp
src/tests/dolphinmainwindowtest.cpp

To: feverfew, #dolphin
Cc: ngraham, elvisangelaccio, anthonyfieroni, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov
Alexander Saoutkin
2018-11-23 18:46:17 UTC
Permalink
feverfew updated this revision to Diff 46088.
feverfew added a comment.


- Make new window option clearer, remove unnecessary string and whitespace

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16648?vs=46087&id=46088

BRANCH
DBusTabInstance

REVISION DETAIL
https://phabricator.kde.org/D16648

AFFECTED FILES
CMakeLists.txt
src/CMakeLists.txt
src/dbusinterface.cpp
src/dolphinmainwindow.cpp
src/dolphinmainwindow.h
src/dolphintabwidget.cpp
src/global.cpp
src/global.h
src/main.cpp
src/tests/dolphinmainwindowtest.cpp

To: feverfew, #dolphin
Cc: ngraham, elvisangelaccio, anthonyfieroni, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov
Alexander Saoutkin
2018-11-23 18:46:59 UTC
Permalink
feverfew updated this revision to Diff 46089.
feverfew added a comment.


- Added braces to one line conditions

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16648?vs=46088&id=46089

BRANCH
DBusTabInstance

REVISION DETAIL
https://phabricator.kde.org/D16648

AFFECTED FILES
CMakeLists.txt
src/CMakeLists.txt
src/dbusinterface.cpp
src/dolphinmainwindow.cpp
src/dolphinmainwindow.h
src/dolphintabwidget.cpp
src/global.cpp
src/global.h
src/main.cpp
src/tests/dolphinmainwindowtest.cpp

To: feverfew, #dolphin
Cc: ngraham, elvisangelaccio, anthonyfieroni, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov
Loading...