D28439: BoorkmarksRunner: Set QIcon instead of Favicon pointer in BookmarkMatch

This revision was automatically updated to reflect the committed changes. Closed by commit R120:c55ffdca923a: BoorkmarksRunner: Set QIcon instead of Favicon pointer in BookmarkMatch (authored by alex). REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE

D28439: BoorkmarksRunner: Set QIcon instead of Favicon pointer in BookmarkMatch

alex added a comment. Friendly Ping :-) REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D28439 To: alex, apol, broulik, davidedmundson Cc: bruns, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot,

D28439: BoorkmarksRunner: Set QIcon instead of Favicon pointer in BookmarkMatch

davidedmundson added a comment. > @davidedmundson - is QIcon(QImage(...)) save? For the way it's used in the runner, yes. > Before this patch the icon was created in the BookmarkMatch::asQueryMatch method which was called from the secondary thread. Then my comment is happily

D28439: BoorkmarksRunner: Set QIcon instead of Favicon pointer in BookmarkMatch

bruns added a comment. @davidedmundson - is `QIcon(QImage(...)) `save? the AppstreamRunner suffers from the same problem, it uses `match->setIcon(/*wrapped*/QIcon::fromTheme())`. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D28439 To: alex, apol,

D28439: BoorkmarksRunner: Set QIcon instead of Favicon pointer in BookmarkMatch

alex added a comment. In D28439#638357 , @bruns wrote: > In D28439#638188 , @davidedmundson wrote: > > > QIcon isn't thread safe. > > > I think this is related to

D28439: BoorkmarksRunner: Set QIcon instead of Favicon pointer in BookmarkMatch

bruns added a comment. In D28439#638188 , @davidedmundson wrote: > QIcon isn't thread safe. I think this is related to https://phabricator.kde.org/D5889?id=14601 ? I think we are save here, as the QIcon is created from an image

D28439: BoorkmarksRunner: Set QIcon instead of Favicon pointer in BookmarkMatch

bruns added a comment. In D28439#638216 , @alex wrote: > In D28439#638188 , @davidedmundson wrote: > > > QIcon isn't thread safe. > > > Sorry, I wasn't aware about that and haven't found

D28439: BoorkmarksRunner: Set QIcon instead of Favicon pointer in BookmarkMatch

alex added a comment. In D28439#638188 , @davidedmundson wrote: > QIcon isn't thread safe. Sorry, I wasn't aware about that and haven't found anything when I had a quick look at the docs before submitting this patch. But if I may

D28439: BoorkmarksRunner: Set QIcon instead of Favicon pointer in BookmarkMatch

davidedmundson added a comment. QIcon isn't thread safe. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D28439 To: alex, apol, broulik, davidedmundson Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2,

D28439: BoorkmarksRunner: Set QIcon instead of Favicon pointer in BookmarkMatch

alex updated this revision to Diff 78906. alex added a comment. Remove unused includes REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28439?vs=78905=78906 BRANCH bookmarks_change_icon_handling_in_matches REVISION DETAIL

D28439: BoorkmarksRunner: Set QIcon instead of Favicon pointer in BookmarkMatch

alex created this revision. alex added reviewers: apol, broulik, davidedmundson. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. alex requested review of this revision. REVISION SUMMARY Before: A pointer to the favicon gets written in the BookmarkMatch and using this