Add mobile interface
Needs ReviewPublic

Authored by Leon0402 on Mar 4 2020, 11:38 AM.

Details

Reviewers
nicolasfella
ngraham
Group Reviewers
Spectacle
VDG
Summary

Adds a mobile interface to Spectacle. It exists beside the desktop interface and is selected based on the
QT_QUICK_CONTROLS_MOBILE environment variable. It was created during KDE SOK 2020.

See https://community.kde.org/SoK/2020/StatusReport/leon0402 for more information

Known issues:

  • Rectangle region mode not optimized for mobile. When should the screenshot be done? On Touch, button press or gesture? Need to adjust the help text then
  • Some warnings regarding Purpose (needs to be fixed in purpose perhaps), a warning that screenshot is null when application is closed (any ideas?)
Test Plan

It is completly untested on KDE mobile, but only tested on kde desktop.

  1. Test if mobile detection works (environment variable)
  2. Try out if all modes and features work

Diff Detail

Repository
R166 Spectacle
Branch
spectacleMobile
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 23305
Build 23323: arc lint + arc unit
Leon0402 created this revision.Mar 4 2020, 11:38 AM
Restricted Application added a project: Spectacle. · View Herald TranscriptMar 4 2020, 11:38 AM
Restricted Application added a subscriber: Spectacle. · View Herald Transcript
Leon0402 requested review of this revision.Mar 4 2020, 11:38 AM
davidre added a subscriber: davidre.Mar 4 2020, 1:40 PM

Nice! First comments after quick look. Will try to find time this week for a more detailed review

First it doesn't load. Because you hardcode the path to "src/MobileFrontend/main.qml" and don't even install the qml files it only works when spectacle is run from the source directory. I don't get the difference between mobile frontend and mobile backend. To me it looks like both are frontend and the backend class does only connect some signals.

The combobox popup doesn't seem to respect the current style?

For the capturemode model: Please don't add the the under mouse mode to shortcut actions. Echoing my other comment, this is an unrelated change and you can just hardcode no shortcut inside your model. How do you imagine the shortcut thing to work on mobile, does it make sense to display shortcuts on mobile?

Also there are quite a few unrelated changes like moving the line braces are in, please clean that up.

src/CMakeLists.txt
49

Why are you adding the heade file here?

src/Main.cpp
41

Why are we always constructing the engine? We could construct it inside SpectacleCore, also drop the braces.

101–102

I don't know if mobile should be another startmode? Couldn't it be the case to have spectacle in dbus mode running on mobile?

src/MobileFrontend/ConfigurationDrawer.qml
1 ↗(On Diff #76925)

Do you need features from Qt 5.14? Or does it work with the current minimum?

Leon0402 updated this revision to Diff 76944.Mar 4 2020, 3:03 PM
  • Change structure

First it doesn't load. Because you hardcode the path to "src/MobileFrontend/main.qml" and don't even install the qml files it only works when spectacle is run from the source directory. I don't get the difference between mobile frontend and mobile backend. To me it looks like both are frontend and the backend class does only connect some signals.

I added a resource file and changed the structure similar to the one used in Okular. I hope that fixes the loading issue?
Regarding the separation of frontend/backend, you're probably right, I put the cpp content in a folder named Components as that fits better. Except maybe the "MobileBackend" file, it's not really a component, but I also agree that it technically isn't really backend. I had some naming problems here. From the "qml code view" it's like a backend, because the qml code talks to it ... but technically it's more a middleware I would say.
I'm open for a different name suggestions and place to put it to.

The combobox popup doesn't seem to respect the current style?

Could you explain more in detail what you mean? It looks fine to me, but not sure.

For the capturemode model: Please don't add the the under mouse mode to shortcut actions. Echoing my other comment, this is an unrelated change and you can just hardcode no shortcut inside your model. How do you imagine the shortcut thing to work on mobile, does it make sense to display shortcuts on mobile?

I'm sorry I didn't want to pass over you other comment ( I also think it's a good idea to finish the other patch, but I wanted to push this first, so it can be tested). The reason I put that in here is that it crashes otherwise, hardcoding no shortcut in the model might be one solution? I will investigate that. In general I think the mode "Window under cursor" doesn't even make sense on mobile, so another idea would be to just remove that mode.
I think, in general it is a very useful feature on mobile too. Creating shortcuts with power button, home button, volume up/down is quite normal on android and iOS as well and I think displaying them makes a lot of sense in general. I'm unsure though, if and how shortcuts are implemented in Kde Mobile.

Thanks for your feedback!

src/CMakeLists.txt
49

It has an enum, which I need to make public to qml. I think therefore, the header file needs to be in here

src/Main.cpp
41

You're probably right. But I think it needs to be a member of SpectacleCode otherwise it will be destructed automatically before app.exec()? I could make it a pointer though, so it is only constructed if it's needed?

101–102

Yes it could be. But I think in that case the dbus startmode can be used as well. Maybe I should rename it to "mobileGui"? It's necessary to decide wether qml code or desktop code is loaded and to set up connections

src/MobileFrontend/ConfigurationDrawer.qml
1 ↗(On Diff #76925)

I doubt that. But isn't it good to use a rather new version, especially for Kirigami, so bugs are fixed?
Where can I find information which version is the recommended minimum?

ngraham requested changes to this revision.Mar 4 2020, 10:44 PM
ngraham added a subscriber: ngraham.

This is really cool!

In terms of the UI, I have some comments:

  • On a phone, the "window under cursor" mode doesn't make sense as there is no cursor. "Active window" mode also probably doesn't make sense as all windows are currently full-screen on Plasma Mobile at least. I suppose on tablets and other platforms there could be multiple app windows open, but this mode should only be visible in that circumstance.
  • I might recommend that the capture mode options be on the main page rather than hidden behind a pop-up panel. This would be easier if the set of available options is reduced. If there are only two, then it could be a two-position slider on the toolbar.
  • The "take new screenshot" button can only be pressed in its top half.
  • The Share pop-up should not be scrollable; rather, it should just resize itself as needed to show all items.

Very nice job!

This revision now requires changes to proceed.Mar 4 2020, 10:44 PM
Leon0402 updated this revision to Diff 77013.Mar 5 2020, 11:39 AM
  • Fix button bug

Thanks a lot Nate!

I agree, the "window under cursor" mode makes no sense. Regarding the "aktive window" mode, I think it makes sense as it doesn't capture the system try? Perhaps the wording is a little bit strange in mobile context, it would rather just be "Capture Window"

I might recommend that the capture mode options be on the main page rather than hidden behind a pop-up panel. This would be easier if the set of available options is reduced. If there are only two, then it could be a two-position slider on the toolbar.

I'm not sure about that, I personally like it the way it is. It looks clean to me and is a flexible approach. I think it could also be possible that more modes are added in future (for example a scrolling screenshot would be extremly nice).

The "take new screenshot" button can only be pressed in its top half.

Fixed, thanks :)

The Share pop-up should not be scrollable; rather, it should just resize itself as needed to show all items.

hmm it's not scrollable for me, but maybe that's because the size is hard coded and your items are bigger, I don't know.
In general, I'm not very happy with that drawer, I think the layout can be improved a lot, but we should then introduce a consistent look to plasma mobile. I like the share popup of apple personally.
I tried to make it resize itself as needed, but it's not so easy as it's a list view and that's not how it is supposed to be used. The list view has an implicit height of zero and creates only the elements visible. But maybe someone has an idea how to archieve that?

Thanks a lot Nate!

I agree, the "window under cursor" mode makes no sense. Regarding the "aktive window" mode, I think it makes sense as it doesn't capture the system try?

Is that something a mobile user would actually care about? Why not just capture the whole screen? Serious question, as I don't really ever take screenshots on my phone so I don't have a strong opinion.

I don't have a strong opinion either. I just thought that it could be of interest to capture only the app itself. Maybe at least for developers?
I think most users are primarly interested in the normal screenshot function and the other features are rather niche. (But it's hided in a popup and it's already there, so I see no particular reason to not have it)