Move the about page from Discover to Kirigami
ClosedPublic

Authored by apol on Nov 28 2018, 2:01 PM.

Details

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

Diff Detail

Repository
R169 Kirigami
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 5474
Build 5492: arc lint + arc unit
apol created this revision.Nov 28 2018, 2:01 PM
Restricted Application added a project: Kirigami. · View Herald TranscriptNov 28 2018, 2:01 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
apol requested review of this revision.Nov 28 2018, 2:01 PM
mart accepted this revision.Nov 28 2018, 2:46 PM
This revision is now accepted and ready to land.Nov 28 2018, 2:46 PM
broulik added inline comments.Nov 28 2018, 2:53 PM
src/controls/AboutPage.qml
103

Not a huge fan of this, privacy-wise, also QtQuick doesn't do any caching on its own

src/controls/LinkButton.qml
36

This thing is missing Accessible and keyboard handling, can you make this style AbstractButton or something instead?

45

That binds to itself?

58

Can this be done using a binding?

src/controls/UrlButton.qml
39

Coding style, spaces

42

Context menus open on press, not click

43

Coding style, braces

52

Add edit-copy icon

src/settings.cpp
136

Can we have this return a QStringList instead instead of assuming this will be rendered as html list? Or is that "not public api"?

src/settings.h
66

Add some docs, also information isn't very descriptive name, also @since

apol updated this revision to Diff 46416.Nov 28 2018, 3:34 PM
apol marked 5 inline comments as done.

Address Kai's comments

apol updated this revision to Diff 46417.Nov 28 2018, 3:35 PM

ADd since

apol marked an inline comment as done.Nov 28 2018, 3:35 PM
apol added inline comments.
src/controls/AboutPage.qml
103

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.

src/controls/LinkButton.qml
36

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.

src/controls/UrlButton.qml
52

I don't think we can have icons in menus yet in Qt 5.9.

leinir added a subscriber: leinir.Nov 29 2018, 12:10 PM
leinir added inline comments.
src/controls/AboutPage.qml
103

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.

src/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 ;)

broulik added inline comments.Nov 29 2018, 12:11 PM
src/controls/AboutPage.qml
103

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"

apol updated this revision to Diff 46476.Nov 29 2018, 2:41 PM

gravatar--

apol closed this revision.Nov 29 2018, 2:55 PM