D17216: Move the about page from Discover to Kirigami
apol closed this revision. REPOSITORY R169 Kirigami REVISION DETAIL https://phabricator.kde.org/D17216 To: apol, #kirigami, mart, broulik Cc: leinir, nicolasfella, plasma-devel, dkardarakos, apol, davidedmundson, mart, hein
D17216: Move the about page from Discover to Kirigami
apol updated this revision to Diff 46476. apol added a comment. gravatar-- REPOSITORY R169 Kirigami CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17216?vs=46417=46476 BRANCH master REVISION DETAIL https://phabricator.kde.org/D17216 AFFECTED FILES kirigami.qrc src/controls/AboutPage.qml src/controls/LinkButton.qml src/controls/UrlButton.qml src/kirigamiplugin.cpp src/qmldir src/settings.cpp src/settings.h To: apol, #kirigami, mart, broulik Cc: leinir, nicolasfella, plasma-devel, dkardarakos, apol, davidedmundson, mart, hein
D17216: Move the about page from Discover to Kirigami
broulik added inline comments. INLINE COMMENTS > leinir wrote in AboutPage.qml:102 > It seems to me that this is not really a privacy issue - it's the developer > emails being leaked here, and those are made all manner of public already. If > it were the end user's gravatar we fetched like this, yeah, maybe not the > best thing, but i think it's ok for this particular purpose. Not talking about the developer's emails, more talking about "this computer is sending a request to that server without the user really knowing which will now know the IP and that they exist" REPOSITORY R169 Kirigami BRANCH master REVISION DETAIL https://phabricator.kde.org/D17216 To: apol, #kirigami, mart, broulik Cc: leinir, nicolasfella, plasma-devel, dkardarakos, apol, davidedmundson, mart, hein
D17216: Move the about page from Discover to Kirigami
leinir added inline comments. INLINE COMMENTS > apol wrote in AboutPage.qml:102 > I do agree it can read a bit weird. > It's generally done on public emails though, I'm not sure why it's bad. > > If we feel better about it, I can drop it and we port it to something else > we're happy with some day. It seems to me that this is not really a privacy issue - it's the developer emails being leaked here, and those are made all manner of public already. If it were the end user's gravatar we fetched like this, yeah, maybe not the best thing, but i think it's ok for this particular purpose. > broulik wrote in settings.h:66 > Add some docs, also `information` isn't very descriptive name, also `@since` i've been trying to come up with a better name than the very generic "information" one, and... well, i've got "versionInformation" and that doesn't really get much better... At the same time, maybe teamed up with the comment below regarding returning a string list rather than a formatted string, perhaps if that is done, then calling it "runtimeVersions" or something to that effect might make sense. Usually i'd go with "if it's difficult to name, you probably did something wrong", but the "something wrong" in this case is that none of the version information is available from within QML, which... well, seems slightly outside the scope of this patch ;) REPOSITORY R169 Kirigami BRANCH master REVISION DETAIL https://phabricator.kde.org/D17216 To: apol, #kirigami, mart, broulik Cc: leinir, nicolasfella, plasma-devel, dkardarakos, apol, davidedmundson, mart, hein
D17216: Move the about page from Discover to Kirigami
apol updated this revision to Diff 46417. apol added a comment. ADd since REPOSITORY R169 Kirigami CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17216?vs=46416=46417 BRANCH master REVISION DETAIL https://phabricator.kde.org/D17216 AFFECTED FILES kirigami.qrc src/controls/AboutPage.qml src/controls/LinkButton.qml src/controls/UrlButton.qml src/kirigamiplugin.cpp src/qmldir src/settings.cpp src/settings.h To: apol, #kirigami, mart, broulik Cc: plasma-devel, dkardarakos, apol, davidedmundson, mart, hein
D17216: Move the about page from Discover to Kirigami
apol marked an inline comment as done. apol added inline comments. INLINE COMMENTS > broulik wrote in AboutPage.qml:102 > Not a huge fan of this, privacy-wise, also QtQuick doesn't do any caching on > its own I do agree it can read a bit weird. It's generally done on public emails though, I'm not sure why it's bad. If we feel better about it, I can drop it and we port it to something else we're happy with some day. > broulik wrote in LinkButton.qml:35 > This thing is missing `Accessible` and keyboard handling, can you make this > style `AbstractButton` or something instead? It used to be an AbstractButton, I remember it having weird behaviors in layouts, and in general we actually want it to behave as a Label. I'll add Accessible. > broulik wrote in UrlButton.qml:51 > Add `edit-copy` icon I don't think we can have icons in menus yet in Qt 5.9. REPOSITORY R169 Kirigami BRANCH master REVISION DETAIL https://phabricator.kde.org/D17216 To: apol, #kirigami, mart, broulik Cc: plasma-devel, dkardarakos, apol, davidedmundson, mart, hein
D17216: Move the about page from Discover to Kirigami
apol updated this revision to Diff 46416. apol marked 5 inline comments as done. apol added a comment. Address Kai's comments REPOSITORY R169 Kirigami CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17216?vs=46406=46416 BRANCH master REVISION DETAIL https://phabricator.kde.org/D17216 AFFECTED FILES kirigami.qrc src/controls/AboutPage.qml src/controls/LinkButton.qml src/controls/UrlButton.qml src/kirigamiplugin.cpp src/qmldir src/settings.cpp src/settings.h To: apol, #kirigami, mart, broulik Cc: plasma-devel, dkardarakos, apol, davidedmundson, mart, hein
D17216: Move the about page from Discover to Kirigami
broulik added inline comments.
INLINE COMMENTS
> AboutPage.qml:102
> +Layout.maximumWidth: Units.iconSizes.medium
> +source: "https://www.gravatar.com/avatar/; +
> Qt.md5(modelData.emailAddress) + "?d=404=" + Units.iconSizes.medium
> +fallback: "user"
Not a huge fan of this, privacy-wise, also QtQuick doesn't do any caching on
its own
> LinkButton.qml:35
> + */
> +QQC2.Label {
> +id: control
This thing is missing `Accessible` and keyboard handling, can you make this
style `AbstractButton` or something instead?
> LinkButton.qml:44
> +
> +font: control.font
> +color: enabled ? Theme.linkColor : Theme.textColor
That binds to itself?
> LinkButton.qml:57
> +
> +onContainsMouseChanged: {
> +control.font.underline = containsMouse && control.enabled
Can this be done using a binding?
> UrlButton.qml:38
> +text: url
> +visible: text.length>0
> +
Coding style, spaces
> UrlButton.qml:41
> +acceptedButtons: Qt.LeftButton | Qt.RightButton
> +onClicked: {
> +if (mouse.button === Qt.RightButton)
Context menus open on //press//, not click
> UrlButton.qml:42
> +onClicked: {
> +if (mouse.button === Qt.RightButton)
> +menu.popup()
Coding style, braces
> UrlButton.qml:51
> +QQC2.MenuItem {
> +text: i18n("Copy link address")
> +onClicked: app.copyTextToClipboard(button.url)
Add `edit-copy` icon
> settings.cpp:136
> +{
> +return tr("KDE Frameworks %1Qt %2 (built against
> %3)The %4 windowing system").arg(
> + QStringLiteral(KIRIGAMI2_VERSION_STRING),
Can we have this return a `QStringList` instead instead of assuming this will
be rendered as html list? Or is that "not public api"?
> settings.h:66
>
> +Q_PROPERTY(QString information READ information CONSTANT)
> +
Add some docs, also `information` isn't very descriptive name, also `@since`
REPOSITORY
R169 Kirigami
BRANCH
master
REVISION DETAIL
https://phabricator.kde.org/D17216
To: apol, #kirigami, mart, broulik
Cc: plasma-devel, dkardarakos, apol, davidedmundson, mart, hein
D17216: Move the about page from Discover to Kirigami
apol created this revision. apol added reviewers: Kirigami, mart, broulik. Herald added a project: Kirigami. Herald added a subscriber: plasma-devel. apol requested review of this revision. REVISION SUMMARY Includes the AboutPage we're using in Discover for other Kirigami projects to use. It also includes UrlButton and LinkButton components that I had in Discover for longtime and can also be useful here. TEST PLAN Used the AboutPage from Discover REPOSITORY R169 Kirigami BRANCH master REVISION DETAIL https://phabricator.kde.org/D17216 AFFECTED FILES kirigami.qrc src/controls/AboutPage.qml src/controls/LinkButton.qml src/controls/UrlButton.qml src/kirigamiplugin.cpp src/qmldir src/settings.cpp src/settings.h To: apol, #kirigami, mart, broulik Cc: plasma-devel, dkardarakos, apol, davidedmundson, mart, hein
