Make using monitor color profile optional
ClosedPublic

Authored by karbie on Apr 17 2020, 1:10 PM.

Details

Summary

BUG: 401154

Diff Detail

Repository
R260 Gwenview
Branch
global_color-management_bugfix
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 26999
Build 27017: arc lint + arc unit
karbie created this revision.Apr 17 2020, 1:10 PM
Restricted Application added a project: Gwenview. · View Herald TranscriptApr 17 2020, 1:10 PM
karbie requested review of this revision.Apr 17 2020, 1:10 PM
ngraham added a subscriber: ngraham.
ngraham added inline comments.
app/advancedconfigpage.ui
293

Typically we phrase checkbox text in the affirmative (e.g. "Use monitor color correction profile"; or even "Respect monitor color correction profile") and then this would be checked by default.

Also ideally this checkbox would have an explanatory label below it like the rendering intent radio buttons to. Not being very familiar with color correction, I have no idea what this checkbox does based on its current label.

karbie updated this revision to Diff 80394.EditedApr 17 2020, 2:37 PM
  • Add better description for color correction setting

English is not my native but I hope the description si clearer now.
So, the idea is the user can choose if they want Gwenview to apply monitor and image color profiles, or only profile from image.
It probably can't be decided by Gwenview, because system-wide color correction might be applied by external software which can make monitor profile applied twice. On the other hand, if there's no system-wide color correction, but monitor profile is set in X11, then applying both gives correct results.

Also, this is my first time collaborating in any FOSS project and especially with KDE there's a lot of new things to watch out for, so kinda expect that I might do something wrong and don't hesitate to point it out.

So sorry I missed your update to this patch! I will re-review.

ngraham requested changes to this revision.May 14 2020, 10:06 PM

Just a few more changes required:

app/advancedconfigpage.ui
265

you'll want to make this text small to match the other help text in the view. To do that, set the name property to something unique (e.g. applyProfileInImageFileHelpLabel and then add a line like so in configDialog.cpp:

mAdvancedConfigPage.applyProfileInImageFileHelpLabel->setFont(QFontDatabase::systemFont(QFontDatabase::SmallestReadableFont));
app/generalconfigpage.ui
10 ↗(On Diff #80394)

unrelated; revert changes to this file

app/imageviewconfigpage.ui
10 ↗(On Diff #80394)

unrelated; revert changes to this file

This revision now requires changes to proceed.May 14 2020, 10:06 PM
karbie updated this revision to Diff 83004.May 16 2020, 6:38 PM
  • make help text smaller
  • stop editing unnecessary files
karbie marked 4 inline comments as done.May 17 2020, 7:48 AM

Sorry to bother, what's the status of this review?

ngraham accepted this revision.May 29 2020, 4:36 PM

All good now, sorry for the wait.

This revision is now accepted and ready to land.May 29 2020, 4:36 PM
ngraham closed this revision.May 29 2020, 4:37 PM