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

Disable restoring hidden quests when there are none to unhide. #5359

Merged
merged 6 commits into from
Jan 16, 2024
Merged

Conversation

jmizv
Copy link
Contributor

@jmizv jmizv commented Nov 5, 2023

The option to unhide hidden quests should be disabled when there are none to unhide.

grafik

grafik

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.

Nice! But, hmm, ... why? Why is it useful to know beforehand how many quests have been hidden?

app/src/main/res/values-de/strings.xml Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
@jmizv jmizv changed the title Show amount of hidden quests and disable restoring hidden quests when… Disable restoring hidden quests when there are none to unhide. Nov 7, 2023
@jmizv
Copy link
Contributor Author

jmizv commented Nov 7, 2023

Nice! But, hmm, ... why? Why is it useful to know beforehand how many quests have been hidden?

Maybe the focus of this PR should be: "Disable the option to unhide hidden quests when there are none". And the information about the currently hidden quests is just a nice-to-have. So it is not useful per se but in combination with the disabling of the resetting it is nice to see why it is disabled (=no hidden quests).

@westnordost
Copy link
Member

Right, okay

@peternewman
Copy link
Collaborator

Maybe the focus of this PR should be: "Disable the option to unhide hidden quests when there are none". And the information about the currently hidden quests is just a nice-to-have. So it is not useful per se but in combination with the disabling of the resetting it is nice to see why it is disabled (=no hidden quests).

I always thought it seemed a bit odd that it told you after you'd unhidden them how many would be unhidden, but not before you made the choice. It seems a nice to have, is it just doing the ten from earlier today or 200 from a few weeks/month?

@jmizv
Copy link
Contributor Author

jmizv commented Jan 8, 2024

Conflicts in .../values-de/strings.xml can be ignored, right?
@westnordost I've made the requested changes. Can you please take a look again? Thanks.

@HolgerJeromin
Copy link
Contributor

Conflicts in .../values-de/strings.xml can be ignored, right?

Your PR should not include additions to translated files. The strings will be translated later.

@jmizv
Copy link
Contributor Author

jmizv commented Jan 8, 2024

Conflicts in .../values-de/strings.xml can be ignored, right?

Your PR should not include additions to translated files. The strings will be translated later.

Okay, conflicts and changes for that file have been removed.

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 good, except for one thing, which is a trifle on its own, but I want to avoid that this kind of thing creeps into the codebase:

Comment on lines 70 to 71
private val osmQuestsHiddenDao: OsmQuestsHiddenDao by inject()
private val noteQuestsHiddenDao: NoteQuestsHiddenDao by inject()
Copy link
Member

Choose a reason for hiding this comment

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

It is uncommon (if not: not done at all) in this codebase to directly access the Daos from view code. Fragments and Activities instead access *Controllers or *Source classes to update or get data. The update of the view subsequently should be reactive to changes in the data, i.e. the quest summary should not update after having tapped the button, but instead the tapping of the button changes the data and because the data is changed, the label is updated.

I wanted to write the details but that got so involved that I rather do it myself unless you know now exactly what to do. I'll do that then otherwise shortly before the next update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd be happy if you could work on this as I don't think I'm expert enough to do it myself. Thank you!

@westnordost westnordost self-assigned this Jan 10, 2024
@westnordost
Copy link
Member

Alright, did this now, I also updated that one overview of classes and how they are connected, this is why the change seems to be so big counted in lines of code. In a nutshell, what I did was:

  • don't access any Dao from anywhere else than the *Controller classes
  • make the OsmQuestController and the OsmNoteQuestController implement new interfaces that are just for handling quest visibility
  • use these interfaces in places where only getting/counting/hiding/unhiding the quests made invisible are of concern - e.g the settings fragment but also the EditHistoryController
  • in the SettingsFragment, update the summary ("x quests hidden") not after clicking that button, but after the data has changed - i.e. the fragment listens now to the changes in the data

@westnordost westnordost merged commit 58c0215 into streetcomplete:master Jan 16, 2024
@westnordost
Copy link
Member

Feel free to look at my changes, you can see them here: 0ab82c5

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.

5 participants