Differential D21416

Add icons for line annotation arrow styles to combo box
ClosedPublic
Actions

Authored by tobiasdeiminger on May 26 2019, 3:51 PM.

Details

Reviewers
sander
Group Reviewers
Okular
Commits
R223:1b89f220ef1b: Add icons for line annotation arrow styles to combo box
Summary

Use LineAnnotPainter to generate accurate icons.

Looks like this:

Depends on D21248

Test Plan
  • open "Edit annotation tool" for straight line and check combo box items for start and stop style
  • test on HiDPI

Diff Detail

Repository
R223 Okular
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
Restricted Application added a project: Okular. · View Herald TranscriptMay 26 2019, 3:51 PM
Restricted Application added a subscriber: okular-devel. · View Herald Transcript
tobiasdeiminger requested review of this revision.May 26 2019, 3:51 PM
tobiasdeiminger edited the summary of this revision. (Show Details)May 26 2019, 3:55 PM
ngraham added a reviewer: Okular.May 26 2019, 4:49 PM
ngraham added a subscriber: ngraham.

Very nice!

ngraham added inline comments.May 28 2019, 3:05 AM
ui/annotationwidgets.cpp
540

Would it make sense to put all of this in a const variable outside of the loop?

davidhurka added a subscriber: davidhurka.May 29 2019, 7:17 PM

The UI looks good and I understand the code. :)

For dark color themes: maybe the color of the adjacent text would be good as foreground color for the icons. Don’t know, but maybe QGuiApplication::pallete().color(QPalette::WindowText)?

tobiasdeiminger updated this revision to Diff 58872.May 29 2019, 9:46 PM

Define TermStyle list outside the loop.

tobiasdeiminger marked an inline comment as done.May 29 2019, 9:59 PM
In D21416#471795, @davidhurka wrote:

For dark color themes: maybe the color of the adjacent text would be good as foreground color for the icons. Don’t know, but maybe QGuiApplication::pallete().color(QPalette::WindowText)?

Makes sense, thanks. Never tried this before, give me some time to check it out...

tobiasdeiminger updated this revision to Diff 58980.Jun 1 2019, 12:43 PM

Consider QGuiApplication::palette, to improve icon visibility in dark themes.

tobiasdeiminger added a comment.Jun 1 2019, 12:59 PM
In D21416#471897, @tobiasdeiminger wrote:
In D21416#471795, @davidhurka wrote:

For dark color themes: maybe the color of the adjacent text would be good as foreground color for the icons. Don’t know, but maybe QGuiApplication::pallete().color(QPalette::WindowText)?

Makes sense, thanks. Never tried this before, give me some time to check it out...

Tried your suggestion, looks good with breeze dark. Would you consider it important to connect to QGuiApplication::paletteChanged, to follow theme changes immediately?

Btw., after a local rebase Phabricator now shows ui/pagepainter.{h,cpp} as part of this D21416, but I actually touched these files only in the parent D21248. Don't know how to correct this.

davidhurka added a comment.Jun 1 2019, 1:19 PM
In D21416#472772, @tobiasdeiminger wrote:
In D21416#471897, @tobiasdeiminger wrote:
In D21416#471795, @davidhurka wrote:

For dark color themes: maybe the color of the adjacent text would be good as foreground color for the icons. Don’t know, but maybe QGuiApplication::pallete().color(QPalette::WindowText)?

Makes sense, thanks. Never tried this before, give me some time to check it out...

Tried your suggestion, looks good with breeze dark. Would you consider it important to connect to QGuiApplication::paletteChanged, to follow theme changes immediately?

Not wrong but not important. How often does one change the color theme while chosing a line ending? I could only imagine that the color theme changes automatically based on enviromnent light sensors, for people who work in a vehicle passing many tunnels. But how often does one chose the line ending while entering/leaving a tunnel the same time?

tobiasdeiminger added a comment.EditedJun 1 2019, 2:51 PM
In D21416#472774, @davidhurka wrote:

Not wrong but not important. How often does one change the color theme while chosing a line ending? I could only imagine that the color theme changes automatically based on enviromnent light sensors, for people who work in a vehicle passing many tunnels. But how often does one chose the line ending while entering/leaving a tunnel the same time?

Yes:) Played around a bit, looks like many widgets in Okular don't react to a theme change immediately (only after shell restart). So implementing paletteChanged for arrow icons would make it worse, unless we fixed the surrounding widgets first.

Guess D21416 is ready now? If someone presses accept (also on D21248), I'll land it.

sander accepted this revision.Jun 1 2019, 2:54 PM
This revision is now accepted and ready to land.Jun 1 2019, 2:54 PM
Closed by commit R223:1b89f220ef1b: Add icons for line annotation arrow styles to combo box (authored by tobiasdeiminger). · Explain WhyJun 1 2019, 3:38 PM
This revision was automatically updated to reflect the committed changes.
bcooksley added a subscriber: bcooksley.Jun 2 2019, 7:43 PM

I'm afraid MSVC doesn't like this change - could someone please take a look?
https://build.kde.org/view/Failing/job/Applications/job/okular/job/kf5-qt5%20WindowsMSVCQt5.11/161/console

This is causing the Dependency Build for Extragear on Windows to fail, so a speedy resolution would be appreciated.
https://build.kde.org/view/Failing/job/Administration/job/Dependency%20Build%20Extragear%20kf5-qt5%20WindowsMSVCQt5.11/

bcooksley added a comment.Jun 2 2019, 8:03 PM

Thanks for the quick update. Unfortunately I don't have a way of easily testing proposed patches, but in theory that should solve it yes.

Revision Contents
Changeset List

PathPackages
Mui/annotationwidgets.h (2 lines)
Mui/annotationwidgets.cpp (42 lines)

Diff 58986

View Options

ui/annotationwidgets.h

Loading...
View Options

ui/annotationwidgets.cpp

Loading...