Remove "Remember last used Save mode" checkbox in Configure window
AbandonedPublic

Authored by acrouthamel on Feb 21 2018, 3:03 PM.

Details

Reviewers
ngraham
Group Reviewers
Spectacle
Summary

Per Nate Graham: "This UI control is a relic from a prior version of spectacle when the "Save" split button's menu was full of items. Now, it only has two, and this setting is checked by default. The use case for turning it off is pretty difficult to envision now. Therefore, in the interest of de-cluttering, let's remove it."

Note: The dynamic Save button still works.

BUG: 390809

Before:

After:

Test Plan

Tested on Kubuntu 17.10 with Backports PPA

KDE Plasma Version: 5.12.1
KDE Frameworks Version: 5.43.0
Qt Version: 5.9.1
Kernel Version 4.13.0-32-generic
OS Type: 64-bit

Diff Detail

Repository
R166 Spectacle
Lint
Lint Skipped
Unit
Unit Tests Skipped
acrouthamel requested review of this revision.Feb 21 2018, 3:03 PM
acrouthamel created this revision.
acrouthamel edited the summary of this revision. (Show Details)
ngraham requested changes to this revision.Feb 21 2018, 3:21 PM
ngraham added a subscriber: rkflx.

Thanks! Great second patch. There's even more code clean-up we can do here, though. For example:

$ grep -ri UseDynamicSaveButton .
./src/SpectacleConfig.cpp:bool SpectacleConfig::useDynamicSaveButton() const
./src/SpectacleConfig.cpp:void SpectacleConfig::setUseDynamicSaveButton(bool enabled)
./src/SpectacleConfig.h:    bool useDynamicSaveButton() const;
./src/SpectacleConfig.h:    void setUseDynamicSaveButton(bool enabled);
Binary file ./src/spectacle matches
./src/Gui/KSMainWindow.cpp:    return cfgManager->useDynamicSaveButton() ? cfgManager->lastUsedSaveMode() : SaveMode::SaveAs;

You can also get rid of QCheckBox *mUseLastSaveAction; on generalOptionsPage.h

Also, make sure you test the upgrade test case:

  • Use Spectacle 17.12.x
  • Build and deploy Spectacle from git master
  • Make sure the save button behaves as expected
This revision now requires changes to proceed.Feb 21 2018, 3:21 PM
acrouthamel abandoned this revision.Feb 21 2018, 3:34 PM

Abandoning this patch due to prior work noted by @rominf

rkflx added a comment.EditedFeb 21 2018, 4:58 PM

@acrouthamel So sorry, this sort of clash is really unfortunate (and the missing integration between Phabricator and Bugzilla does not help either…). You were off to a good start, though!

Did you find something else to work on in the meantime? If not, how about those (let me know if you start on something, so we can coordinate better):

Gwenview:

Spectacle:

@rkflx No worries. I'm currently messing with Bug 390737. If you have any tips on what builds that tree menu in kmenuedit, let me know. :) I'm just starting out, so I haven't figured out the code yet.

I'll bookmark your bug suggestions as well.