Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow for tracking session to be resumed (inc. across QField restarts) #4590

Merged
merged 17 commits into from
Oct 1, 2023

Conversation

nirvn
Copy link
Member

@nirvn nirvn commented Sep 20, 2023

This PR adds the infrastructure to resume tracking sessions (including across QField reboots/restarts). When a tracking session has previously been started, QField now offers to resume the last session:

image

Practically, it means that for lines and polygons, the created feature from the previous session is carried over.

In order to be able to restore tracking sessions across QField launches, or pause/resume tracks within a same session, we need to decouple the tracking settings UI from the tracking session item. Doing so actually makes the handling of tracks a little bit easier to understand, and avoids mixing UI and core elements together.

The newly added TrackerSettings.qml is essentially the popup part of TrackingSession.qml taken out.

I've taken the time to clean up the tracking model a fairbit and add missing data() roles as well as guarding against invalid row index.

@qfield-fairy
Copy link
Collaborator

qfield-fairy commented Sep 20, 2023

@nirvn nirvn force-pushed the tracker_settings_move branch 2 times, most recently from fc3a4f7 to 245dfff Compare September 20, 2023 10:41
@nirvn nirvn changed the title Dissociate tracking session item from tracker settings popup Allow for tracking session to be resumed (inc. across QField restarts) Sep 20, 2023
Copy link
Collaborator

@suricactus suricactus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some styling comments

src/core/geometry.cpp Outdated Show resolved Hide resolved
src/core/projectinfo.h Outdated Show resolved Hide resolved
src/core/projectinfo.h Outdated Show resolved Hide resolved
src/core/tracker.h Show resolved Hide resolved
src/qml/TrackerSettings.qml Outdated Show resolved Hide resolved
src/qml/TrackerSettings.qml Outdated Show resolved Hide resolved
src/qml/TrackerSettings.qml Outdated Show resolved Hide resolved
src/qml/TrackerSettings.qml Outdated Show resolved Hide resolved
src/qml/TrackerSettings.qml Outdated Show resolved Hide resolved
src/qml/TrackerSettings.qml Outdated Show resolved Hide resolved
@suricactus
Copy link
Collaborator

suricactus commented Sep 29, 2023

Few comments on the UI:

  • I would make the popup without margins on the side or fullscreen. At least on vertical mobile is no nice.
  • The groupings Time/Distance/Sensor constraints are not super clear. Why don't we use tabs or another way of grouping? The distance constraints settings that appear after enabling the toggle are not indented. Probably adding bold on the parent groups will improve the situation?
  • Without documentation for me it is super unclear what the terms are meaning. E.g. in the QField Settings things do have meaning. But I am not sure if the Tracking Settings use coherent terms as the QField Settings.
  • Not directly related can be in follow-up PR. We had to go to docs to see the tracking button appears only if the positioning is enabled. I guess we should keep the "Tracking" button on vector layers active and disable only the "Start tracking" in the popup in case the position is off.
  • Another idea that came was to rename "Setup Tracking" to just "Tracking", so you don't think you have to go all the way. In the layer settings we can add a button submenu with "Start Tracking" and "Continue Tracking", so it is more discoverable.
  • I thought in Qt6 we have fixed the huge bottom padding in popups, but I see a lot of space after "Continue Tracking".

@lindacamathias can you test before me merge all of this, I am pretty sure you are more familiar with the context and will provide better user feedback.

@nirvn
Copy link
Member Author

nirvn commented Sep 29, 2023

@suricactus , thanks for the review, I've added a bunch of missing documentation in projectinfo.h on top of the functions hinted at in your review.

Regarding popup styling (and other possible UI improvements), I suggest we defer to another PR, this one is quite disruptive as it is. I don´t think we'd go fullcreen on this one as for tablets it'd look weird for both orientation, however we can reduce margins for narrow screen width.

I'd be -1 on tabs in this UI in part because I think we want to keep most settings super quick to access. I have been trying to think of a way to visually regroup settings into themes (e.g. her we could have Constraints and General groups), it'd be useful here and in the settings panel. I haven't settled on the way it should be done visually yet though.

I'm seeing margins for constraints and erroneous position sub-parameters here.

If you have a photo of the bottom padding issue on Qt6, could you attach here?

@nirvn
Copy link
Member Author

nirvn commented Sep 29, 2023

(note that the client sponsoring this feature has done some testing already, which revealed a couple of regressions that have been addressed)

@nirvn nirvn mentioned this pull request Oct 1, 2023
Copy link
Collaborator

@suricactus suricactus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the PR and speedy update of the UI. Looks perfect now!

@nirvn
Copy link
Member Author

nirvn commented Oct 1, 2023

@suricactus , thanks for the crucial pair of eyes and suggestions, made a huge difference.

@nirvn nirvn merged commit f638ddd into master Oct 1, 2023
19 checks passed
@nirvn nirvn deleted the tracker_settings_move branch October 1, 2023 06:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants