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

refactor: DialogHandler #13359

Merged
merged 1 commit into from
Mar 6, 2023

Conversation

david-allison
Copy link
Member

@david-allison david-allison commented Feb 28, 2023

Move serialisation + handling to classes instead of being spread around the codebase

Keep enum: WhichDialogHandler to ensure that we do not reuse int values

Pull Request template

Purpose / Description

  • Our DialogHandler implementation split our two aspects of handling Messages (which isn't the easiest code to follow):
    • Serialisation around the codebase
    • Deserialisation + logic in DialogHandler

Related

Approach

  • Split out each DialogHandler into a class: containing the method and the serde of its parameters
  • Document the class a little better
  • Use an enum to ensure that no items have duplicated what integers, and all cases are handled

How Has This Been Tested?

Unit test only

Learning (optional, can help others)

Finally documenting our use of Message: The implicit assumption is that we launch all of the dialogs from DeckPicker, and the notification uses an intent of DeckPicker, so everything should work. This assumption occasionally breaks down

Checklist

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

@david-allison david-allison added the Review High Priority Request for high priority review label Feb 28, 2023
Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

LGTM

@mikehardy mikehardy added the Needs Second Approval Has one approval, one more approval to merge label Feb 28, 2023
@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label Mar 6, 2023
@david-allison
Copy link
Member Author

Rebased

Move serialisation + handling to classes instead of being in 2/3 places
(DialogHandler/DeckPicker/Dialog Class)

Keep enum: `WhichDialogHandler` to ensure that we do not reuse int values
@david-allison david-allison removed the Needs Author Reply Waiting for a reply from the original author label Mar 6, 2023
@mikehardy
Copy link
Member

A week is long enough for folks to have a chance, "go"!

@mikehardy mikehardy merged commit d6bbea7 into ankidroid:main Mar 6, 2023
@github-actions github-actions bot added this to the 2.16 release milestone Mar 6, 2023
@github-actions github-actions bot removed Review High Priority Request for high priority review Needs Second Approval Has one approval, one more approval to merge labels Mar 6, 2023
@david-allison david-allison deleted the refactor-dialoghandler branch March 6, 2023 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

2 participants