Skip to content
This repository has been archived by the owner on Apr 24, 2024. It is now read-only.

Introduce map polygon. #1085

Merged
merged 80 commits into from
Dec 9, 2023
Merged

Introduce map polygon. #1085

merged 80 commits into from
Dec 9, 2023

Conversation

badnames
Copy link
Member

@badnames badnames commented Nov 24, 2023

Basics

  • I added a line to changelog.md
  • The PR is rebased with current master.
  • Details of what you changed are in commit messages.
  • References to issues, e.g. close #X, are in the commit messages and changelog.
  • The buildserver is happy.

Checklist

  • I have installed and I am using pre-commit hooks
  • I fully described what my PR does in the documentation
  • I fixed all affected documentation
  • I fixed the introduction tour
  • I wrote migrations in a way that they are compatible with already present data
  • I fixed all affected decisions
  • I added automated tests or a manual test protocol
  • I added code comments, logging, and assertions as appropriate
  • I translated all strings visible to the user
  • I mentioned every code or binary not directly written or done by me in reuse syntax
  • I created left-over issues for things that are still to be done
  • Code is conforming to our Architecture
  • Code is conforming to our Guidelines
  • Code is consistent to our Design Decisions
  • Exceptions to any guidelines are documented

Review

  • I've tested the code
  • I've read through the whole code
  • I've read through the whole documentation
  • I've checked conformity to guidelines

@badnames badnames self-assigned this Nov 24, 2023
@badnames badnames linked an issue Nov 24, 2023 that may be closed by this pull request
@markus2330
Copy link
Contributor

jenkins build please

1 similar comment
@badnames
Copy link
Member Author

badnames commented Dec 4, 2023

jenkins build please

Copy link
Contributor

@markus2330 markus2330 left a comment

Choose a reason for hiding this comment

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

  • icons at bottom
  • tooltip for icons

@markus2330
Copy link
Contributor

markus2330 commented Dec 4, 2023

When moving polygon Points its possible to select the entire map geometry via mouse drag.

This is not really a big problem. Would of course be nice.

  • A bigger problem is that for new points the nearest point to any line should be used, not to the end-points of a line.
  • Another problem is that changing between moving and adding is tiresome. It would be great if move/add can be done via middle click or with a modifier while clicking.

Maybe @danielsteinkogler can help with these two points, they are be needed for drawing layer, too.

@badnames don't forget to review #1110 😉 Did you see the review request?

@badnames
Copy link
Member Author

badnames commented Dec 5, 2023

Another problem is that changing between moving and adding is tiresome. It would be great if move/add can be done via middle click or with a modifier while clicking.

We should definitely avoid overloading middle click, as that is currently used for panning the map.
What do you think about using simple keycombinations (e.g. Ctrl.-Shift-A for adding, Ctrl.-Shift-D for deleting and Ctrl.-Shift-M for moving points) instead?
Also, if we do use this approach we should consider reserving a certain combination of modifier keys (like Ctrl.-Shift) for layer specific operations to create a more streamlined UX and simplify our code.

Copy link
Contributor

@markus2330 markus2330 left a comment

Choose a reason for hiding this comment

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

Super-nice, I was able to do the polygon for our garden in a few minutes without any annoyance. ❤️

But there is still the problem that by default you don't see the bounding box. Please make at least the default bounding box smaller or as suggested by Yvonne (ideal solution) make the default zoom when entering a map as big as the bounding box. Can also be a follow-up.

backend/src/service/map.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@danielsteinkogler danielsteinkogler left a comment

Choose a reason for hiding this comment

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

I really like your code, it is very understandable and i didn't find any obvious issues. great job 👍

@markus2330
Copy link
Contributor

What do you think about using simple keycombinations (e.g. Ctrl.-Shift-A for adding, Ctrl.-Shift-D for deleting and Ctrl.-Shift-M for moving points) instead?

This should be done but not instead (it is also lower-prior). The UX is not ideal due to the constant need to change between add+move. It would be better if there is only one add/move mode and when clicking on a polygon point it gets moved and when click somewhere else a new one gets created. Can also be follow-up.

Bounding box zoom fix or follow-up missing, too.

@absurd-turtle can you also take a look?

@badnames
Copy link
Member Author

badnames commented Dec 6, 2023

What do you think about using simple keycombinations (e.g. Ctrl.-Shift-A for adding, Ctrl.-Shift-D for deleting and Ctrl.-Shift-M for moving points) instead?

This should be done but not instead (it is also lower-prior). The UX is not ideal due to the constant need to change between add+move. It would be better if there is only one add/move mode and when clicking on a polygon point it gets moved and when click somewhere else a new one gets created. Can also be follow-up.

Bounding box zoom fix or follow-up missing, too.

@absurd-turtle can you also take a look?

In that case I would still very much prefer modifier keys, as I could imagine that having both move and add enabled at the same time would be pretty finicky and even more annoying than having to switch edit modes constantly.
Especially for beginners or users that don't have great fine motor skills.

@markus2330
Copy link
Contributor

I looked again how others are doing it and I didn't see a tool that combines add and move functionality, so it is probably not the best idea. What exists is that shift creates Bezier-help-points. But this is not needed for the base layer. Keybindings, however, should be added.

So pls create 2 follow-up issues, then we can hopefully merge this PR soon.

@badnames
Copy link
Member Author

badnames commented Dec 9, 2023

Left-over issues are #1112 and #1113.

@markus2330 markus2330 merged commit e56d8df into master Dec 9, 2023
@markus2330 markus2330 deleted the feature/816-introduce-map-polygon branch December 9, 2023 15:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce map polygon. Set borders of map (frontend)
3 participants