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

Add QField camera UI to toggle geotagging #4685

Merged
merged 1 commit into from
Oct 24, 2023

Conversation

nirvn
Copy link
Member

@nirvn nirvn commented Oct 24, 2023

This PR adds a UI to the QField camera to toggle geotagging:

image

When enabled, the following tags are added:

  • latitude, longitude, and altitude (provided they are valid values)
  • ground speed
  • compass orientation

@nirvn nirvn added this to the 3.1 milestone Oct 24, 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.

Nice functionality! Code looks good, did not test myself. Very small comment on colors.

EDIT: once again, nice bird!

anchors.topMargin: 4

iconSource: positionSource.active ? Theme.getThemeIcon("ic_geotag_24dp") : Theme.getThemeIcon("ic_geotag_missing_24dp")
iconColor: positionSource.active && settings.geoTagging ? Theme.mainColor : "white"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
iconColor: positionSource.active && settings.geoTagging ? Theme.mainColor : "white"
iconColor: positionSource.active && settings.geoTagging ? Theme.mainColor : Theme.lightColor

Isn't it better to avoid mixing themed and hardcoded colors?

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case, not really. I mean, we could have Theme.white, but I feel like it's not gaining much.

This specific color doesn't change across our light/dark theme either.

If we want to say have something like Theme.defaultToolButttonColor (being white) I suggest we do that in a separate PR as it'll touch a dozen QML files or so.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, the idea is that if we decide to add another color theme in the future, we are not sure if the white will remain white. Probably your suggestion to have Theme.defaultToolButttonColor is even better. Lets merge like this for now.

@qfield-fairy
Copy link
Collaborator

@nirvn nirvn merged commit 5517fbd into master Oct 24, 2023
20 of 21 checks passed
@nirvn nirvn deleted the move_orientation_into_positioning branch October 24, 2023 14:06
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