Skip to content

Allow multiple access values per key in access manager#887

Open
mcliquid wants to merge 11 commits intoHelium314:modifiedfrom
mcliquid:feature/access-values-multiple-choice-651
Open

Allow multiple access values per key in access manager#887
mcliquid wants to merge 11 commits intoHelium314:modifiedfrom
mcliquid:feature/access-values-multiple-choice-651

Conversation

@mcliquid
Copy link
Copy Markdown

That was a lot of work, but I think I managed to solve it pretty well. The code is certainly not perfect, but so far it works in my tests. Please feel free to give feedback or test it!

What changed?

  • Multi-select for all access keys now possible
  • Clean OSM serialization (value1;value2)
  • Existing values parsed correctly
  • Overview shows only set values
  • Intuitive removal of individual values
  • Fixed dialog crash

Screen Record

Because of the video size available here: https://clip.place/w/uXmCrRNVeTTFmSqVGC1rom
I tried to cover most combinations.

Screenshots

image image image image

Closes #651

@mcliquid
Copy link
Copy Markdown
Author

After testing the new features here a few times myself, one more idea for further discussion:
Key-specific access values
Should the quest offer only a limited selection of values for certain keys? For example, the wiki explains that access=designated is not useful at all, that dismount should only be used for bicycle, or that use_sidepath should only be used for foot/bicycle.
To do this, I could perhaps create a allowedValuesByKey list in a separate accessValues.kt with permitted values per key.
For example:

val allowedValuesByKey: Map<String, List<String>> = mapOf(
    "access" to listOf("yes", "no", "private", "permissive", "permit", "destination", "delivery", "customers"),
    "foot" to listOf("yes", "no", "designated", "use_sidepath", "dismount", "permissive"),
    "bicycle" to listOf("yes", "no", "designated", "use_sidepath", "dismount", "permissive"),
    "motor_vehicle" to listOf("yes", "no", "destination", "delivery", "private"),
)

Of course, this would severely restrict the freedom of choice through strict requirements. If we trust our users, there is no need for this.

@mnalis
Copy link
Copy Markdown
Collaborator

mnalis commented Feb 18, 2026

If we trust our users, there is no need for this.

Yeah, probably no need. It would IMHO needlessly add complexity and restrictions.

And we need to trust them anyway, because if they don't know what specific keys and values (e.g. ohv or permit) actually mean, they are going to mess up the database in any case; restrictions or not.

For hiking trails mostly
@Helium314
Copy link
Copy Markdown
Owner

Sorry to be slow to answer.
I had a short look when the PR was fresh, but actually wasn't quite happy because a few days before I had mostly migrated the conditional dialog to compose, but fully switching also requires switching the access manager (I finally committed the changes to the conditional dialog yesterday, but kept the old variant because switching the access manager would break this PR).
Now either I have to fully review and understand the changes in this PR just to reimplement them right away in compose, or throw away what was a lot of work for you... and none of this is a good option.

Not sure what's the best way to avoid unnecessary work for both of us.
(please in the future avoid PRs that change Android views, at least without first asking)

@mcliquid
Copy link
Copy Markdown
Author

Thanks for the heads-up, fully agree with you. My work on this PR mainly involves getting a basic understanding of the dialog, researching the access keys in the wiki and taginfo, and above all, testing. None of that would be wasted in my view, if I close this PR, the Compose-based access manager is implemented, and I then try to code the new features into the updated dialog.
Maybe I’m stepping back unnecessarily now, but I feel that your time investment in this app is more significant than mine.
What do you think? Close this one, wait for the final implementation and rework the logic into Compose?

@Helium314
Copy link
Copy Markdown
Owner

I added a compose AccessManagerDialog, but kept the old one for now, might make it easier for you.
Once the PR is merged I'll remove the Android dialogs (and make the compose dialog also appear in overlays)

@mcliquid
Copy link
Copy Markdown
Author

I moved the functional changes from the old Android dialog into app\src\commonMain\kotlin\de\westnordost\streetcomplete\osm\AccessManagerDialog.kt locally.

The Compose AccessManagerDialog now supports the same main behavior that was previously implemented in the Android version and that is in short:

  • expanded accessKeys
  • multiple values per access key instead of a single selection
  • parsing existing semicolon-separated access values into sets
  • serializing the selected values back into stable semicolon-separated strings on save
  • adding and editing multiple values through a multi-select dialog
  • removing individual values, and removing the whole key when no values remain
  • conditional access entries still added through the Compose AddConditionalDialog

The dialog state was changed from Map<String, String> to a set-based representation, so existing tags (not days, DeepL ;-) ) like access=delivery;customers can now be displayed and edited correctly. The save logic still produces a normal StringMapChangesBuilder, so the integration with the existing form flow stays unchanged.

Here's a short test, app on current modifed-branch - If this is heading in the right direction, I can push the changes here.
https://github.com/user-attachments/assets/0e620f23-7687-4595-a561-3d2d93600963

@Helium314
Copy link
Copy Markdown
Owner

Looks good!
There are some minor things, but I'll comment on the code.

@mcliquid
Copy link
Copy Markdown
Author

mcliquid commented Apr 1, 2026

damn, sorry for the commit hiccup. Should I create a new PR with just the Compose-File changes?

@Helium314
Copy link
Copy Markdown
Owner

Should I create a new PR with just the Compose-File changes?

No need, you can also remove the changes to the old dialog here.

On the changes (just tested, without really reviewing the code):

  • I think the checkboxes and per-key add buttons don't really fit. Better remove them, and move the bin to work per-value instead of per-key.
  • the key should be better aligned with the values
  • a divider between the key would help separating keys when there are many values
  • the multi-select dialog is not scrollable (same for the access manager, but that's my fault)

@mcliquid
Copy link
Copy Markdown
Author

mcliquid commented Apr 2, 2026

Changes are implemented, I tested it only in the Emulator and that one is really slow. I will try the next days on my physical phone outside.

Screen_recording_20260402_220447.mp4

@mcliquid
Copy link
Copy Markdown
Author

mcliquid commented Apr 2, 2026

hm ok - launched it on physical device, the scrolling in the value- and manage list is terrible. it's jumping wild around. no clue why.

@Helium314
Copy link
Copy Markdown
Owner

Looks good, except for the scrolling

@Helium314
Copy link
Copy Markdown
Owner

For scrolling possibly the ScrollableAlertDialog helps.

@mcliquid
Copy link
Copy Markdown
Author

mcliquid commented Apr 3, 2026

Thanks for the hint, changed the implementation, jumping is gone 🚀
First thoughts (current state):

  • The selection dialog for the access values and later the manage access dialog could be higher compared to the add access key dialog, which has full height of screen.
  • The technical tagging is working fine
  • selection of multiple values is working fine
  • divider is added
  • bin icon is added
  • padding is decreased (less gab between items)

I will have a look again this weekend and can finalize the PR if more feedback is incoming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Access values multiple choice

3 participants