Change UI for setting jpeg quality to slider
ClosedPublic

Authored by MrPepe on Mon, Sep 30, 12:35 PM.

Details

Summary

D23106 added a spinner box to set a save quality for JPEG images. I changed it to a slider to be consistent with the same functionality in Spectacle, but I didn't use the ticks because it makes the slider me misaligned with the respective label. Also, I used % as the label unit. I think it is more clear and will therefore propose the same for Spectacle.

I did not really understand how in the original commit the value from the spinner box was saved to the config, so I set it explicitly when the slider value changes.

Test Plan

Set high quality in settings, don't change in save dialog -> Output image has high quality
Set high quality in settings, override in save dialog with low quality -> Output image has low quality
Set low quality in settings, don't change in save dialog -> Output image has low quality
Set low quality in settings, override in save dialog with high quality -> Output image has high quality

Diff Detail

Repository
R260 Gwenview
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
MrPepe created this revision.Mon, Sep 30, 12:35 PM
Restricted Application added a project: Gwenview. · View Herald TranscriptMon, Sep 30, 12:35 PM
MrPepe requested review of this revision.Mon, Sep 30, 12:35 PM
ngraham requested changes to this revision.Mon, Sep 30, 8:48 PM

On one hand, a slider looks nicer and it's easier to set gross values, but the lack of a text input field makes it harder to enter precise values. I wonder if we should replace the label that's next to the slider with an text field. That way we'd get the best of both worlds (and then we'd do this for Spectacle too).

Giving the object the name kcfg_JPEGQuality invokes some behind-the-scenes magic such that the connection between the slider and a config value with the name "kcfg_JPEGQuality" is made automatically. So you don't need to add a new function. Please remove that and make sure that your patch still works (it should).

This revision now requires changes to proceed.Mon, Sep 30, 8:48 PM
ngraham added a subscriber: Spectacle.
MrPepe updated this revision to Diff 67096.Tue, Oct 1, 12:58 AM
MrPepe added a comment.Tue, Oct 1, 1:01 AM

Removed the explicit call to GwenviewConfig::setJPEGQuality().
Still works.

Should the change for using a text field go here as well or in a separate revision?

MrPepe updated this revision to Diff 67101.Tue, Oct 1, 5:19 AM

Replaced the Label with a SpinBox

Tests: Set the quality using the SpinBox -> Also works

ngraham accepted this revision.Tue, Oct 1, 7:53 PM

Thanks, this is lovely. Much better. Can you provide an email address so I can land the patch with correct git authorship information?

Also, would you be interested in sending a patch for Spectacle to implement the same UI there? Goal: Consistency :)

This revision is now accepted and ready to land.Tue, Oct 1, 7:53 PM
MrPepe added a comment.Wed, Oct 2, 1:07 AM

I'm in China and the arcanist download takes forever and then fails.

E-Mail: mr-peipei@web.de
Name: Felipe Peter

And yes, I will submit a patch for Spectacle as well ;)

This revision was automatically updated to reflect the committed changes.