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

Multiple collection times on the same weekdays as selected before #2109

Merged
merged 3 commits into from
Sep 25, 2020

Conversation

kmpoppe
Copy link
Collaborator

@kmpoppe kmpoppe commented Sep 24, 2020

Closes #2108.

Quest for adding collection times to amenity=post_box nodes was changed.

Just like the quest for adding opening times, this quest will now, if there already is a row, ask if you want to add a collection time to the same weekday(s) as the row before or if you want to add more weekdays.

CollectionTimesAdapter.addNew() was intentionally left in the code (I'm not sure if Android Studio correctly finds all references) bur calls addNewWeekdays(). If this should be removed, please advise @westnordost.

Copy link
Member

@westnordost westnordost left a comment

Choose a reason for hiding this comment

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

Looks fine, thank you! I just have a few small comments.

Did you test it?

@kmpoppe
Copy link
Collaborator Author

kmpoppe commented Sep 24, 2020

Looks fine, thank you! I just have a few small comments.

Will change them in the morning, thanks.

Did you test it?

I tested it on my phone from the Dev app that Android studio runs on the phone, yes. The UI works fine, but I don't know how to test if the changes really check out in reality. What's your way of checking if then changes you made create the desired result in the OSM Data?

@westnordost
Copy link
Member

I tested it on my phone from the Dev app that Android studio runs on the phone, yes. The UI works fine, but I don't know how to test if the changes really check out in reality. What's your way of checking if then changes you made create the desired result in the OSM Data?

I run the app without being logged in, then I solve a quest and look at the log in Android Studio (filter by "QuestController" to remove the noise), there you can see what will be tagged.

@kmpoppe
Copy link
Collaborator Author

kmpoppe commented Sep 24, 2020

I run the app without being logged in, then I solve a quest and look at the log in Android Studio (filter by "QuestController" to remove the noise), there you can see what will be tagged.

Nice touch, thanks! Next enhancement: display a message box with the changes if app is in debug mode 😬

@westnordost
Copy link
Member

Right, good idea, I implemented it. It is in the settings menu -> show quest forms (last entry)

@kmpoppe
Copy link
Collaborator Author

kmpoppe commented Sep 25, 2020

Did you test it?

image
image

Looks good to me. Thanks for the great testing method, make sure the other Contributors get to know about it, it'll be very handy!

@kmpoppe kmpoppe requested a review from westnordost September 25, 2020 03:37
@kmpoppe
Copy link
Collaborator Author

kmpoppe commented Sep 25, 2020

@westnordost I just realized that you already put
and (!collection_times or collection_times older today -${r * 2} years)
in the tagFilter, without there being a proper resurvey question operation. Do you want to keep it that way for now (leads to a form asking for the collection times without displaying the currently tagged ones)?

@westnordost
Copy link
Member

Yes. It makes sense to revisit collection times every few years. To implement that previous collection times are shown was something I deemed to be too much effort for the gain. See #2105

@westnordost
Copy link
Member

Looks good, merging!

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.

Allow for multiple postbox collection times on the same day
2 participants