Add option to choose which view to close
ClosedPublic

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

Details

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
angeloevertonjr created this revision.Jan 7 2019, 2:29 PM
Restricted Application added a project: Dolphin. · View Herald TranscriptJan 7 2019, 2:29 PM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
angeloevertonjr requested review of this revision.Jan 7 2019, 2:29 PM
angeloevertonjr retitled this revision from # Close inactive split view when toggling off to Close inactive split view when toggling off.Jan 7 2019, 2:30 PM
angeloevertonjr edited the summary of this revision. (Show Details)
angeloevertonjr edited the summary of this revision. (Show Details)Jan 7 2019, 2:32 PM
angeloevertonjr edited the test plan for this revision. (Show Details)Jan 8 2019, 12:39 PM
ngraham requested changes to this revision.Jan 8 2019, 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.Jan 8 2019, 11:53 PM
cfeck added a subscriber: cfeck.Jan 9 2019, 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.Jan 9 2019, 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.Jan 9 2019, 12:53 AM
src/dolphintabpage.cpp
103
if (...) {
    ....
} else {
   ....
}
cfeck added inline comments.Jan 9 2019, 12:55 AM
src/dolphintabpage.cpp
103

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

elvisangelaccio requested changes to this revision.Jan 12 2019, 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
91–107

We can just drop this comment

src/settings/dolphin_generalsettings.kcfg
77

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.Jan 12 2019, 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.EditedJan 13 2019, 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.Jan 13 2019, 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?

@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?

Just add m_closeActiveSplitView->setToolTip(i18n("Some descriptive text here"));

As for the checkbox text itself, how about this? "Turning off split view closes active pane"

Fixing broken diff

angeloevertonjr marked an inline comment as done.Feb 12 2019, 8:45 AM

ToolTip creation

angeloevertonjr added a comment.EditedFeb 12 2019, 8:52 AM

ToolTip done, waiting for feedback. Any more change request?

Ps: Also... maybe it's a silly question, but regarding code style... should I move the setToolTip line to between new QCheckBox and topLayout->addRow lines?

ngraham accepted this revision.Feb 14 2019, 10:56 PM

I would move your new setToolTip() up one line, yeah. But just as a general principle, we strive to copy the formatting of the existing code.

The behavior and UX is now perfect to me. Code generally looks sane too. @elvisangelaccio, what do you think?

elvisangelaccio accepted this revision.Feb 16 2019, 3:15 PM

Looks good to me now, landing it.

Thanks for the patch!

This revision is now accepted and ready to land.Feb 16 2019, 3:15 PM
This revision was automatically updated to reflect the committed changes.