D23241: Show application window icon on AboutPage
This revision was automatically updated to reflect the committed changes. Closed by commit R169:59a9b92fc6c7: Show application window icon on AboutPage (authored by caspermeijn, committed by ngraham). REPOSITORY R169 Kirigami CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D23241?vs=64512=71258 REVISION DETAIL https://phabricator.kde.org/D23241 AFFECTED FILES src/controls/AboutPage.qml src/settings.cpp src/settings.h To: caspermeijn, mart, apol, #kirigami, hein Cc: bcooksley, sitter, ngraham, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, apol, ahiemstra, davidedmundson, mart, hein
D23241: Show application window icon on AboutPage
mart accepted this revision. mart added a comment. It looks quite weird to have it in something called "Settings", tough.. i' fine (in kf6 should really be renamed to something more sensible) I agree with Eike that it would be better for it to be in Qt.Application, but i'm fine with it for now REPOSITORY R169 Kirigami BRANCH about_icon (branched from master) REVISION DETAIL https://phabricator.kde.org/D23241 To: caspermeijn, mart, apol, #kirigami, hein Cc: bcooksley, sitter, ngraham, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, apol, ahiemstra, davidedmundson, mart, hein
D23241: Show application window icon on AboutPage
caspermeijn added a comment. Can I do anything for this to get merged? REPOSITORY R169 Kirigami BRANCH about_icon (branched from master) REVISION DETAIL https://phabricator.kde.org/D23241 To: caspermeijn, mart, apol, #kirigami, hein Cc: bcooksley, sitter, ngraham, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, apol, davidedmundson, mart, hein
D23241: Show application window icon on AboutPage
fsitter added a comment. You think sending your minions to insult me and then disabling my account will solve the issue? what will you do next? Disable registration so no one points out your hypocrisy? Which overlord made the decision to disable my account and for what? REPOSITORY R169 Kirigami BRANCH about_icon (branched from master) REVISION DETAIL https://phabricator.kde.org/D23241 To: caspermeijn, mart, apol, #kirigami, hein Cc: fsitter, sitter, ngraham, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, apol, davidedmundson, mart, hein
D23241: Show application window icon on AboutPage
sitter added inline comments. INLINE COMMENTS > caspermeijn wrote in settings.h:81 > I have no idea what the value would be for that and Kirigami doesn't have any > example. https://doc.qt.io/qt-5/qtqml-cppintegration-definetypes.html#type-revisions-and-versions tldr: - add `REVISON 10` to property - change krigiamiplugin.cpp to register that revision I am not sure how that would work for a singletontype though, one always needs all properties since any revision may be used at any time. @apol are you sure this makes sense for singletons? REPOSITORY R169 Kirigami BRANCH about_icon (branched from master) REVISION DETAIL https://phabricator.kde.org/D23241 To: caspermeijn, mart, apol, #kirigami, hein Cc: sitter, ngraham, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, apol, davidedmundson, mart, hein
D23241: Show application window icon on AboutPage
caspermeijn marked an inline comment as done. caspermeijn added inline comments. INLINE COMMENTS > apol wrote in settings.h:81 > It could make sense to include the REVISION part: > https://doc.qt.io/qt-5/properties.html I have no idea what the value would be for that and Kirigami doesn't have any example. REPOSITORY R169 Kirigami BRANCH about_icon (branched from master) REVISION DETAIL https://phabricator.kde.org/D23241 To: caspermeijn, mart, apol, #kirigami, hein Cc: ngraham, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, apol, davidedmundson, mart, hein
D23241: Show application window icon on AboutPage
caspermeijn updated this revision to Diff 64512. caspermeijn added a comment. Fixed since version number REPOSITORY R169 Kirigami CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D23241?vs=63975=64512 BRANCH about_icon (branched from master) REVISION DETAIL https://phabricator.kde.org/D23241 AFFECTED FILES src/controls/AboutPage.qml src/settings.cpp src/settings.h To: caspermeijn, mart, apol, #kirigami, hein Cc: ngraham, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, apol, davidedmundson, mart, hein
D23241: Show application window icon on AboutPage
hein added a comment. I was looking into this recently and added the component named getter, which does the trick for me and is much nicer to use than setting the program logo. So in some sense this is not strictly necessary, and also it would really be better if the window icon first part of the Qt.application upstream API in QML. It's still an okay workaround though. Let's see what Marco thinks. REPOSITORY R169 Kirigami BRANCH about_icon (branched from master) REVISION DETAIL https://phabricator.kde.org/D23241 To: caspermeijn, mart, apol, #kirigami, hein Cc: ngraham, plasma-devel, fbampaloukas, domson, dkardarakos, apol, davidedmundson, mart, hein
D23241: Show application window icon on AboutPage
apol added a comment. +1 LGTM, please address the @since bit INLINE COMMENTS > settings.h:81 > + */ > +Q_PROPERTY(QVariant applicationWindowIcon READ applicationWindowIcon > CONSTANT) > It could make sense to include the REVISION part: https://doc.qt.io/qt-5/properties.html REPOSITORY R169 Kirigami REVISION DETAIL https://phabricator.kde.org/D23241 To: caspermeijn, mart, apol, #kirigami Cc: ngraham, plasma-devel, fbampaloukas, domson, dkardarakos, apol, davidedmundson, mart, hein
D23241: Show application window icon on AboutPage
ngraham added a reviewer: Kirigami. ngraham added inline comments. INLINE COMMENTS > settings.h:79 > + * @since 5.62 > + * @since org.kde.kirigami 2.9 > + */ 2.10 actually (this is super confusing, I know) REPOSITORY R169 Kirigami REVISION DETAIL https://phabricator.kde.org/D23241 To: caspermeijn, mart, apol, #kirigami Cc: ngraham, plasma-devel, fbampaloukas, domson, dkardarakos, apol, davidedmundson, mart, hein
D23241: Show application window icon on AboutPage
caspermeijn added a comment. I am not sure about the version numbers and the return type. Did I choose the correct ones? REPOSITORY R169 Kirigami REVISION DETAIL https://phabricator.kde.org/D23241 To: caspermeijn, mart, apol Cc: plasma-devel, fbampaloukas, domson, dkardarakos, apol, davidedmundson, mart, hein
D23241: Show application window icon on AboutPage
caspermeijn created this revision. Herald added a project: Kirigami. Herald added a subscriber: plasma-devel. caspermeijn requested review of this revision. REVISION SUMMARY With this it is not nessecary to KAboutData::setProgramLogo. If the QApplication::setWindowIcon is set, then the AboutPage will automatically use that icon. TEST PLAN Tested with Discover, with only setWindowIcon, only setProgramLogo and both setWindowIcon and setProgramLogo REPOSITORY R169 Kirigami BRANCH about_icon (branched from master) REVISION DETAIL https://phabricator.kde.org/D23241 AFFECTED FILES src/controls/AboutPage.qml src/settings.cpp src/settings.h To: caspermeijn Cc: plasma-devel, fbampaloukas, domson, dkardarakos, apol, davidedmundson, mart, hein
