Differential D19738

Fix "Add Network Folder" tooltip icon does not show on Breeze, shows correctly on Breeze Dark
ClosedPublic
Actions

Authored by hallas on Mar 13 2019, 8:13 PM.

Details

Reviewers
elvisangelaccio
ngraham
Group Reviewers
Dolphin
Commits
R318:508dc815c75c: Fix "Add Network Folder" tooltip icon does not show on Breeze, shows correctly…
Summary

Fix "Add Network Folder" tooltip icon does not show on Breeze, shows correctly
on Breeze Dark. The fix is taken from D19596.

Test Plan

Open Dolphin with the Breeze theme
Hover the mouse over the "Add Network Folder"
The icon is black on black

BUG: 404858

Diff Detail

Repository
R318 Dolphin
Branch
fix_add_network_folder_icon_color (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 9548
Build 9566: arc lint + arc unit
hallas created this revision.Mar 13 2019, 8:13 PM
Restricted Application added a project: Dolphin. · View Herald TranscriptMar 13 2019, 8:13 PM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
hallas requested review of this revision.Mar 13 2019, 8:13 PM
ngraham added a comment.Mar 13 2019, 8:29 PM

This works great! But why do we need a KIconLoader singleton?

broulik added a subscriber: broulik.Mar 13 2019, 10:15 PM

I suppose since creating a KIconLoader instance is quite heavy (parses all the theme directories and so on) we surely don't want to do that every time a tooltip is requested.
(However, isn't ToolTipManager a singleton of its own already?)

hallas added a comment.Mar 14 2019, 5:20 AM

Honestly I didn't give it much thought, I basically "ported" the fix from D19596. But I can see that ToolTipManager is not a singleton, it is dynamically allocated by DolphinView, so I think it is a good idea to keep KIconLoader as a singleton so that we keep a single instance across all Dolphin views.

ngraham accepted this revision as: ngraham.Mar 15 2019, 10:01 PM

The approach makes sense to me and I can confirm that it works. @elvisangelaccio?

This revision is now accepted and ready to land.Mar 15 2019, 10:01 PM
elvisangelaccio accepted this revision.Mar 17 2019, 5:56 PM

We definitively don't want to create a KIconLoader every time the user hovers a file.

hallas added a comment.Mar 17 2019, 6:03 PM

@elvisangelaccio - Should this be pushed to Applications/19.04 and then merged to master ? Or only pushed to master?

elvisangelaccio added a comment.Mar 17 2019, 6:07 PM
In D19738#433042, @hallas wrote:

@elvisangelaccio - Should this be pushed to Applications/19.04 and then merged to master ? Or only pushed to master?

Please push to Applications/19.04 and then merge to master.

hallas closed this revision.Mar 17 2019, 6:11 PM
hallas added a comment.Mar 17 2019, 6:14 PM
In D19738#433049, @elvisangelaccio wrote:
In D19738#433042, @hallas wrote:

@elvisangelaccio - Should this be pushed to Applications/19.04 and then merged to master ? Or only pushed to master?

Please push to Applications/19.04 and then merge to master.

Done :D

Hope I did it correct this time

Revision Contents
Changeset List

Diff 53829

View Options

src/views/tooltips/tooltipmanager.cpp

Loading...