Tells you which preset you have chosen via shortcut
ClosedPublic

Authored by WhaleKit on Aug 21 2017, 1:51 PM.

Details

Summary

After changing brush using shortcuts "Next Favorite Preset", "Previous Favorite Preset" and "Switch to Previous Preset" (by default binded to keys: " , ", " . " and " / ") it might be not obvious, which preset just have been selected.

This patch makes so, that after selecting another preset using one of theese shortcuts, selected preset's name and thumbnail will be shown in floating message.

Here's what it looks like:
https://i.imgur.com/WIdcD9Z.png

Diff Detail

Repository
R37 Krita
Lint
Lint Skipped
Unit
Unit Tests Skipped
WhaleKit created this revision.Aug 21 2017, 1:51 PM
Restricted Application added a subscriber: woltherav. · View Herald TranscriptAug 21 2017, 1:51 PM
WhaleKit edited the summary of this revision. (Show Details)Aug 21 2017, 1:52 PM
WhaleKit edited the summary of this revision. (Show Details)Aug 21 2017, 1:54 PM
rempt accepted this revision.Aug 21 2017, 5:49 PM

I'm not sure why you changed the structure of the loops, but that's fine. Otherwise, the patch works as advertised :-) Shall I push for you?

This revision is now accepted and ready to land.Aug 21 2017, 5:49 PM
In D7454#138233, @rempt wrote:

I'm not sure why you changed the structure of the loops, but that's fine. Otherwise, the patch works as advertised :-)

I feel like usage of range-based for-loops makes sence, when counter variable by itself doesn't matter. Since here counter variable is used (not just as index to access element in single container), it makes more sence to use "classic" for loop that have counter variable in it.

Shall I push for you?

Yes, please.

The code seems ok. We might want to simplify the wording a bit on the floating message, but that is easy enough to do later. The brush name should be shown first as that is the most important piece of information. Something like 'preset 1 selected'

rempt added a comment.Aug 22 2017, 9:47 AM

Yes, I think that "Current preset: " is enough. Other than that, please get a developer account and push your patch :-)

See https://community.kde.org/Infrastructure/Get_a_Developer_Account

Correct i18n for the floating message should be something like i18n("Current preset changed to: %1\n", preset->name())

https://techbase.kde.org/Development/Tutorials/Localization/i18n_Mistakes

Nice patch.

Perhaps we could skip the entire commenting text? Just keep the preset name
only? The icon of the preset clearly tells the user that this text is the
preset name

WhaleKit updated this revision to Diff 18619.Aug 23 2017, 6:37 PM

Brush name comes first. If I left only brush name, the text would be 1 line.
For some reason when text is 1 line height, the brush thumbnail in floating message is shown too small.

This revision was automatically updated to reflect the committed changes.