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

fix: "Reviewing" & Deck import do not handle SchedV1 correctly #17857

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

oyeraghib
Copy link
Contributor

@oyeraghib oyeraghib commented Jan 22, 2025

Purpose / Description

"Reviewing" & Deck import do not handle SchedV1 correctly

Fixes

Approach

created a function to handle the update of SchedV1 inside Reviewing in Preferences Settings

How Has This Been Tested?

Physical device : Manually tested on Pixel 6a - Android 15

Before

screen-20250130-023513-2.mp4

After

screen-20250130-022757-2.mp4

Learning (optional, can help others)

Checklist

Please, go through these checks before submitting the PR.

  • 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

@@ -100,38 +116,73 @@ class PreferencesFragment :
caller: PreferenceFragmentCompat,
pref: Preference,
): Boolean {
// Checks if the user needs to upgrade the scheduler before opening Reviewing
if (pref.fragment == ReviewingSettingsFragment::class.jvmName) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you do the check in the reviewing fragment itself? This looks messy

Copy link
Contributor Author

@oyeraghib oyeraghib Jan 26, 2025

Choose a reason for hiding this comment

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

@BrayanDSO That could be done. But isn't this should be the ideal behaviour that "ReviewFragment" is not opened if the SchedV1 is not updated. If this is put in ReviewFragment, that would open the fragment even though nothing would be functional until Sched is updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. It definitely looks much cleaner.

@mikehardy mikehardy added the Needs Author Reply Waiting for a reply from the original author label Jan 26, 2025
@oyeraghib oyeraghib force-pushed the develop/v1-scheduler-upgrade branch from f5cfe55 to f7c33b6 Compare January 29, 2025 21:01
@oyeraghib oyeraghib requested a review from BrayanDSO January 29, 2025 21:15
@BrayanDSO BrayanDSO removed the Needs Author Reply Waiting for a reply from the original author label Feb 5, 2025
@BrayanDSO
Copy link
Member

Since nothing should work on a V1 collection, I think that the dialog should be shown when entering the app (maybe in the database error dialog) and shouldn't be dismissible.

That way we won't have the issue again in the future because we missed a screen, or because something new doesn't work on sched v1.

It also should be simpler to code.

@BrayanDSO BrayanDSO added Needs Author Reply Waiting for a reply from the original author Needs reviewer reply Waiting for a reply from another reviewer labels Feb 5, 2025
@oyeraghib
Copy link
Contributor Author

@BrayanDSO I tried implementing that and it works fine. But the problem is in one of the case -> When the app has already opened the app and imports a v1 collection then in that case there won't be any checks that time as the app wasn't started so no dismissible dialog will be shown and we will have to handle it like this only.

Let me know what you think here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Author Reply Waiting for a reply from the original author Needs reviewer reply Waiting for a reply from another reviewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Settings - "Reviewing" & Deck import do not handle SchedV1 correctly
3 participants