D17216: Move the about page from Discover to Kirigami

[Aleix Pol Gonzalez]

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

[Kai Uwe Broulik]

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&s=" + 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("<ul><li>KDE Frameworks %1</li><li>Qt %2 (built against %3)</li><li>The <em>%4</em> windowing system</li></ul>").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

[Aleix Pol Gonzalez]

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&id=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

[Aleix Pol Gonzalez]

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&id=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

[Aleix Pol Gonzalez]

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

[Dan Leinir Turthra Jensen]

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

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

[Kai Uwe Broulik]

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

[Aleix Pol Gonzalez]

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&id=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

[Aleix Pol Gonzalez]

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