Add option to choose which view to close
Needs ReviewPublic

Authored by angeloevertonjr on Mon, Jan 7, 2:29 PM.

Details

Reviewers
ngraham
elvisangelaccio
Group Reviewers
Dolphin
Summary

This Diff make configurable which view will close when toggling off the split view mode, if it's the active one or the inactive one. A new checkbox was added to the Dolphin configuration window, and defaults to the original behavior.

FEATURE: 312834

Test Plan

Diff Detail

Repository
R318 Dolphin
Branch
split-view-close-inactive
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 7035
Build 7053: arc lint + arc unit
angeloevertonjr created this revision.Mon, Jan 7, 2:29 PM
Restricted Application added a project: Dolphin. · View Herald TranscriptMon, Jan 7, 2:29 PM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
angeloevertonjr requested review of this revision.Mon, Jan 7, 2:29 PM
angeloevertonjr retitled this revision from # Close inactive split view when toggling off to Close inactive split view when toggling off.Mon, Jan 7, 2:30 PM
angeloevertonjr edited the summary of this revision. (Show Details)
angeloevertonjr edited the summary of this revision. (Show Details)Mon, Jan 7, 2:32 PM
angeloevertonjr edited the test plan for this revision. (Show Details)Tue, Jan 8, 12:39 PM
ngraham requested changes to this revision.Tue, Jan 8, 11:53 PM
ngraham added a reviewer: Dolphin.

Thanks! I feel that implementing this is important. 312834 is one of the most common bug reports against Dolphin, with five duplicates and a lot of unhappy user comments. I think we should listen to our users and land this feature.

I notice one issue that needs to be addressed before I feel comfortable making that recommendation though: Dolphin needs to update the conditions for when it changes the icon displayed in the button so that it accurately shows which view will be closed. This code is in`DolphinMainWindow::updateSplitAction()`

This revision now requires changes to proceed.Tue, Jan 8, 11:53 PM
cfeck added a subscriber: cfeck.Wed, Jan 9, 12:41 AM

The commit headline is wrong. You are actually adding an option to choose, not changing the behavior to close the inactive view.

Please also remove the historical background from the commit description. If the headline "Add option to choose which view to close" is not descriptive enough, add a detailed explanation in the description.

cfeck added inline comments.Wed, Jan 9, 12:52 AM
src/settings/general/behaviorsettingspage.cpp
104

I guess we need to come up with a better description, "when toggling off" sounds confusing. Or could that just be removed, and a more verbose explanation added to a tooltip? Those are also accessible via Qt Accessibility.

cfeck added inline comments.Wed, Jan 9, 12:53 AM
src/dolphintabpage.cpp
110
if (...) {
    ....
} else {
   ....
}
cfeck added inline comments.Wed, Jan 9, 12:55 AM
src/dolphintabpage.cpp
110

Maybe in this case a ternary ? : is even more readable, but I am not sure of maintainers preference.

elvisangelaccio requested changes to this revision.Sat, Jan 12, 3:47 PM
elvisangelaccio added a subscriber: elvisangelaccio.

For the record I still think that we should add another button rather than a new option. But anyway I think we discussed this enough, let's go with the option.

That said, commit message should say "FEATURE" and not "BUG", see https://community.kde.org/Policies/Commit_Policy#Special_keywords_in_GIT_and_SVN_log_messages

src/dolphintabpage.cpp
98

We can just drop this comment

src/settings/dolphin_generalsettings.kcfg
73

Please call the entry "CloseActiveSplitView" rather than "CloseFocusedSplitView". Same in the all the new variables you added.
We use "active" instead of "focused" all over the place, so we should stick to that naming convention.

angeloevertonjr retitled this revision from Close inactive split view when toggling off to Add option to choose which view to close.Sat, Jan 12, 9:28 PM
angeloevertonjr edited the summary of this revision. (Show Details)
angeloevertonjr edited the summary of this revision. (Show Details)
angeloevertonjr updated this revision to Diff 49367.EditedSun, Jan 13, 12:18 AM

Requests attended

  • All references of "focused" changed to "active"
  • Split view button icon fixed for each option
  • Minor fixes in code style
angeloevertonjr marked 4 inline comments as done.Sun, Jan 13, 12:21 AM

@ngraham Thanks for the feedback, I didn't even noticed that the button icon changes dynamically, and without you I would probably delay much more to find which code to change in that immense file. I made your requested change but I got into another bug, when the configuration is updated with split view enabled, the button stays with the old icon until the active view changes to a new one, it's a minor bug but still worth, I'll separate a time for it later.

@cfeck Could you provide more details about the desired changes? When I move the mouse over the checkboxes, no tooltip balloon appears or something like that, so what (and where) should I add exactly?