Port towards KConfig XT
ClosedPublic

Authored by davidre on Aug 21 2019, 1:08 PM.

Details

Summary

Port settings to Kconfig XT. This enables us to drop our own settings class and
our own configuration dialog logic with it's logic. Setting the objectNames of
the Widgets accordingly enables us to reuse them.
The settings not managed through the Dialog (last save locations and screenshot
options in the main window) are written now on exit but are still instant apply.
Using this opportunity this also unifies the naming style and moves some
settings inside the config file around: In general they are now in the same group
as they are in the settings dialog. Additionally [Save] includes
lastSave(As)Location, too. In [GuiConfig] are the options which are set in the
main window and the last crop region.
Also includes a behavioral change: "Open Screenshots Folder" will now open the
default folder as configured. The folder with the the last saved screenshot can
be opened from the inline message when saving.

BUG: 389894
FIXED-IN: 19.12.0

Test Plan

Works as before but the dialog has now more buttons

Diff Detail

Repository
R166 Spectacle
Branch
arcpatch-D23316
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 19540
Build 19558: arc lint + arc unit
davidre created this revision.Aug 21 2019, 1:08 PM
Restricted Application added a project: Spectacle. · View Herald TranscriptAug 21 2019, 1:08 PM
davidre requested review of this revision.Aug 21 2019, 1:08 PM
davidre updated this revision to Diff 64675.Aug 26 2019, 4:15 PM

Rebase add autocopy and autosave

davidre retitled this revision from [WIP] Port towards KConfig XT to Port towards KConfig XT.Aug 26 2019, 4:16 PM
davidre edited the summary of this revision. (Show Details)
ngraham edited the summary of this revision. (Show Details)Aug 26 2019, 8:52 PM
  • The settings window has some graphical glitches:
  • All my settings were restored to their default values
  • The Press Screenshot To radio button did not have a pre-selected value
  • PNG images are huge again
  • The settings window has some graphical glitches:

Whoops that was just for debugging

  • All my settings were restored to their default value
  • The Press Screenshot To radio button did not have a pre-selected value
  • PNG images are huge again

Did you do the update? I think I need to still add the installation of it to the CmakeLists.txt

Ah yeah, probably best to add that last, yeah.

davidre updated this revision to Diff 64738.Aug 27 2019, 1:28 PM
  • Install update file and make workaround work

The dialog still needs to be resized so every page fits if there is no way to do it automatically. Looking at Okular maybe not since it also has some scrollable pages?

I don't believe there's an automatic way. Generally people just set a minimum size on the parent dialog that's high enough to accommodate the tallest and widest page.

davidre updated this revision to Diff 64812.Aug 28 2019, 10:02 AM
  • Set a size for the dialog
davidre updated this revision to Diff 69504.Nov 9 2019, 2:35 PM

Use real file

Maybe we can do this for 20.03 with 19.12 now branched. This would leave us some time to test it. What do you think @ngraham?

@crossi, @ervin would you mind reviewing this? I saw you porting all the KCMs to KConfig XT and this is the first time I came in contact with the system.

ervin added a comment.Nov 18 2019, 7:31 AM

Looks good to me regarding the kconfig_compiler use (epsilon one key which would be better suited to an enum).
The next natural step would be to switch the dialog pages to ui files, that would remove some more code.

src/CMakeLists.txt
46

Probably doesn't make sense to install that one indeed.

src/Gui/KSMainWindow.cpp
264

Smells like you could have used an enum for that one.

src/Gui/SettingsDialog/spectacle.kcfg
114

Why not use Enum for that one? Doesn't look like something which would be properly managed by a uint.

crossi added inline comments.Nov 18 2019, 1:26 PM
src/Gui/KSMainWindow.cpp
463–466

Style related, consider putting return statement in a new line.
Also, KDE code style requires to use curly braces even when the body of a conditional statement contains only one line.

src/Gui/SettingsDialog/spectacle.kcfg
24

Default element is optional, but this entry is the only one that has no default value, is it meant ?

Looks good to me regarding the kconfig_compiler use (epsilon one key which would be better suited to an enum).
The next natural step would be to switch the dialog pages to ui files, that would remove some more code.

If we have to port it to anything, I'd prefer a QML-based UI rather than making new .ui files.

ervin added a comment.Nov 18 2019, 5:05 PM

Looks good to me regarding the kconfig_compiler use (epsilon one key which would be better suited to an enum).
The next natural step would be to switch the dialog pages to ui files, that would remove some more code.

If we have to port it to anything, I'd prefer a QML-based UI rather than making new .ui files.

I'll have to disagree here for several reasons:

  1. the rest of the application is widget based and it's not going to change soon, pulling QML will make it fatter; 1bis) it will increase quite a bit the complexity of an otherwise rather small application;
  2. you get more for free on the widget base KConfigXT use than the QML one (especially for immutable keys that we have no way to automate on the QML side);
  3. this is a desktop app with a desktop look, there's no particular gain switching to QML.

There's no good justification for porting to QML imo.

davidre updated this revision to Diff 70089.Nov 21 2019, 9:58 AM
davidre marked 5 inline comments as done.

address comments

LGTM, I'd still aim for the .ui port as a second step though.

davidre updated this revision to Diff 70109.Nov 21 2019, 3:11 PM
  • Fix typo in update file
davidre updated this revision to Diff 70154.Nov 22 2019, 12:19 PM

Also use enum for the last selected captureMode

This additonaly fixes a bug in the Combobox if you switched between Wayland and X.
Because it stored the index and the number of items in the combobox is less on
Wayland it could happen that no item was selected when switching from X to
Wayland. I assume the original author thought that addItem would honor the
indices but they are just used to decide if the item is prepended or
appended.

davidre updated this revision to Diff 70334.Nov 26 2019, 9:49 AM

Correctly read default value dependent on another config entry

davidre updated this revision to Diff 70953.Dec 5 2019, 1:45 PM

Need a bit more height

ngraham requested changes to this revision.Dec 5 2019, 10:36 PM

All functionality works perfectly for me. However this regressed the default width of the settings window. Not it's not wide enough to accommodate all controls without an ugly horizontal scrollbar:

This revision now requires changes to proceed.Dec 5 2019, 10:36 PM
davidre added a comment.EditedDec 6 2019, 8:17 AM

All functionality works perfectly for me. However this regressed the default width of the settings window. Not it's not wide enough to accommodate all controls without an ugly horizontal scrollbar:

Hmm interesting. Do you run anything different to default settings? For me it looks like this:


Youre font seems bolder? Anyways we can easily increase the height.

davidre updated this revision to Diff 71001.Dec 6 2019, 8:20 AM

Rebase once more

Youre font seems bolder? Anyways we can easily increase the height.

I'm using a different (Ubuntu), larger (11pt), and darker (#000000) font. However the current Spectacle's config window takes this into account and makes the window the correct minimum width (the height is fine). With this patch, the minimum width isn't high enough anymore. Not sure why.

Is there a way we can move forwards? Do other Applications that use KConfigDialog exhibit this problem, too? If yes maybe it's something to improve at that level. If not we can have a look what they do.
https://lxr.kde.org/ident?_i=KConfigDialog
For example in Okular I even get scrollbars with default settings.

Look good in openSUSE:

Operating System: openSUSE Tumbleweed 20191214
KDE Plasma Version: 5.17.4
KDE Frameworks Version: 5.64.0
Qt Version: 5.13.1
Kernel Version: 5.3.12-1-default
OS Type: 64-bit
Processors: 4 × Intel® Core™ i7-8550U CPU @ 1.80GHz
Memory: 31.1 GiB

My Frameworks version is a bit old. Maybe @ngraham is using the Git version.

Saving option is broken on my system. Clicking "Save" button throws error "Invalid file name". The default pattern is not saved. I have to go to configuration dialog and click "OK" to save the file name pattern.

The default format combobox shows "BMP". But it was "PNG" before. For screenshots, I think "PNG" and "JPEG" are enough. More options bring confusion.

Saving option is broken on my system. Clicking "Save" button throws error "Invalid file name". The default pattern is not saved. I have to go to configuration dialog and click "OK" to save the file name pattern

Do you mean it's not saved in the config file? That's fine, the generated code doesn't write default values to the config file.
I can't reproduce the error with an empty or my spectaclerc. Can you reproduce it? With you current config file or with a new one? If yes, what's your value of saveFilenameFormat?

The default format combobox shows "BMP". But it was "PNG" before. For screenshots, I think "PNG" and "JPEG" are enough. More options bring confusion.

That's because I changed the key in the config file. See the update script

With empty spectaclerc it is fine. I forget to keep my original rc file. Probably have a back up in my external drive. Will let you know when I get the backup.

ngraham accepted this revision.Dec 20 2019, 7:29 PM

Is there a way we can move forwards? Do other Applications that use KConfigDialog exhibit this problem, too? If yes maybe it's something to improve at that level. If not we can have a look what they do.
https://lxr.kde.org/ident?_i=KConfigDialog
For example in Okular I even get scrollbars with default settings.

Right, and this is something we get bug reports on: https://bugs.kde.org/show_bug.cgi?id=397678

I guess since that's a pre-existing issue, we shouldn't block this on it. But since the port exposes Spectacle to that issue, I think we'll need to fix it soon.

This revision is now accepted and ready to land.Dec 20 2019, 7:29 PM

Landing it now - it's after christmas and I should have time in the next days/week to fix issues that we find.

This revision was automatically updated to reflect the committed changes.

Landing it now - it's after christmas and I should have time in the next days/week to fix issues that we find.

But then the commit message is incorrect, as it's not FIXED-IN 19.12.

I pushed a few follow-up fixes to the strings introduced by this change here: https://commits.kde.org/spectacle/befcb1748b2a770bce28d3aed0f0b4addd06d622

Landing it now - it's after christmas and I should have time in the next days/week to fix issues that we find.

But then the commit message is incorrect, as it's not FIXED-IN 19.12.

I pushed a few follow-up fixes to the strings introduced by this change here: https://commits.kde.org/spectacle/befcb1748b2a770bce28d3aed0f0b4addd06d622

Thanks! That fixed in slipped my mind probably because it was so long on phabricator. I will adjust the bug manually

broulik reopened this revision.Jan 13 2020, 8:54 AM
broulik added a subscriber: broulik.

This breaks saving screenshots with keyboard shortcuts.

Press Meta+Shift+PrtScr, select a region, hit return. Nothing happens, no screenshot is saved, no notification appears.

This revision is now accepted and ready to land.Jan 13 2020, 8:54 AM
broulik requested changes to this revision.Jan 13 2020, 8:55 AM
This revision now requires changes to proceed.Jan 13 2020, 8:55 AM
broulik accepted this revision.Mar 11 2020, 10:11 AM
This revision is now accepted and ready to land.Mar 11 2020, 10:11 AM
davidre closed this revision.Mar 11 2020, 10:15 AM