Rename Transparent background config options
ClosedPublic

Authored by huoni on Mar 31 2018, 12:00 AM.

Details

Summary

This makes the meaning of this option more clear. It also uses the
correct term "checkerboard" instead of "check board".

I have left the RasterImageView::AlphaBackground* terminology the same
as I believe it makes more sense as is in terms of what the code does.
For example AlphaBackgroundNone means we do not draw a background or
draw a transparent background.

Depends on D11630

Before:

After:

Test Plan

Ensure all transparent background options continue to function.

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.
huoni requested review of this revision.Mar 31 2018, 12:00 AM
huoni created this revision.
huoni edited the summary of this revision. (Show Details)Mar 31 2018, 12:01 AM
huoni added a project: Gwenview.

See https://phabricator.kde.org/D11630#233271 for the initial discusison.

Maybe we should call the "none" option "General background", since then it will hint to people that it's the "Background" setting on the general page? Otherwise I'm pretty okay with "Image View background".

rkflx added a comment.Mar 31 2018, 5:53 PM

Maybe we should call the "none" option "General background", since then it will hint to people that it's the "Background" setting on the general page? Otherwise I'm pretty okay with "Image View background".

Haha, got the same idea earlier today: General background color

it's the "Background" setting on the general page?

Only in non-fullscreen mode :) which is why I still advocate for "No background". To me that implies that it will always blend in to what the viewport background is including the texture in fullscreen.

It could be "No special background" to make it more clear?

+1 for "No special background"

rkflx added a comment.Apr 1 2018, 8:05 AM

+1 for "No special background"

Hm, it's more of an anti-pattern if we detail what it is not, which we should avoid.

As for General background color, "general" could also be understood as "standard" or "surrounding", so I don't think it will pose too much of a problem. The reference to the other config page is only an additional property, and if at all the UI problem is on that config page, i.e. as mentioned elsewhere making the fullscreen background colour configurable too would be one option.


More ideas: Page background, Screen background, View background

huoni added a comment.Apr 1 2018, 8:08 AM

understood as "standard" or "surrounding"

What about Match surrounding background?

rkflx added a comment.Apr 1 2018, 8:10 AM

understood as "standard" or "surrounding"

What about Match surrounding background?

I like it ;) Slightly clashing with the "with" in front, though, so perhaps only Surrounding background?

huoni added a comment.Apr 1 2018, 8:17 AM

I like it ;) Slightly clashing with the "with" in front, though, so perhaps only Surrounding background?

Oh yeah, the label. Yep I can get behind Surrounding background.

"Surrounding background" it is, then!

huoni updated this revision to Diff 31129.Apr 1 2018, 10:29 PM
  • Image View background -> Surrounding background
huoni edited the summary of this revision. (Show Details)Apr 1 2018, 10:31 PM
huoni updated this revision to Diff 31130.Apr 1 2018, 10:34 PM
  • Change code as well
huoni added inline comments.Apr 1 2018, 10:35 PM
app/imageviewconfigpage.ui
38

Should I be adding an & for this option since the other two have one?

S is taken though, so R?

huoni updated this revision to Diff 31133.Apr 1 2018, 10:55 PM
  • Rebase
rkflx accepted this revision.Apr 2 2018, 7:44 AM

Yay, finally we found something!

app/imageviewconfigpage.ui
38

Well, in a150e6263573 I wrote

with the remaining accelerators at the start of words as a deliberate choice

…but here the "S" is already taken, so you'd need to pick a different letter. On the other hand Qt will already pick a letter for you, and for other languages this won't work anyway.

Personally I would not bother adding the &.

This revision is now accepted and ready to land.Apr 2 2018, 7:44 AM
This revision was automatically updated to reflect the committed changes.