Skip to content

Conversation

@criticalAY
Copy link
Contributor

@criticalAY criticalAY commented Jan 31, 2025

Purpose / Description

Approach

  • Create a new activity to handle the logged in and !LoggedIn situation as we are starting the fragments from preference screen
  • Two different fragments to handle the two situations i.e. Logged in and vice versa
  • Use view model for Login fragment and handles the changes via the viewmodel
  • remove the old code
  • update existing test (one more test file is needed for Logged in UI which I deleted)

How Has This Been Tested?

Test On API 35 Google emulator:
I am uploading all the screenshots and videos to drive as it would be too much for the template

https://drive.google.com/drive/folders/1lYg4rz-SdFhwb-3rlQIs5aUwnjwJgDiK?usp=sharing

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

@criticalAY criticalAY force-pushed the frag-migrate-myaccount branch 7 times, most recently from 7306b07 to 25e9a69 Compare February 1, 2025 13:14
@mikehardy
Copy link
Member

Please test locally before pushing

@criticalAY criticalAY force-pushed the frag-migrate-myaccount branch from 25e9a69 to dde04fc Compare February 1, 2025 13:32
Copy link
Member

@BrayanDSO BrayanDSO left a comment

Choose a reason for hiding this comment

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

How Has This Been Tested?
Test On API 35

Quoting the PR template:

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration (SDK version(s), emulator or physical, etc)

So please do that. Your description isn't enough for anyone to reproduce or trust your testing process. A video would be even better.

@BrayanDSO BrayanDSO added the Needs Author Reply Waiting for a reply from the original author label Feb 1, 2025
@criticalAY criticalAY force-pushed the frag-migrate-myaccount branch 2 times, most recently from c36fd3f to 033deeb Compare February 1, 2025 18:41
@criticalAY
Copy link
Contributor Author

I have uploaded all the SSs to drive along with few test videos

@criticalAY criticalAY added Needs Review and removed Needs Author Reply Waiting for a reply from the original author labels Feb 1, 2025
@BrayanDSO BrayanDSO added Needs Author Reply Waiting for a reply from the original author Needs Review and removed Needs Review Needs Author Reply Waiting for a reply from the original author labels Feb 3, 2025
@BrayanDSO BrayanDSO self-requested a review February 5, 2025 13:59
Copy link
Member

@BrayanDSO BrayanDSO left a comment

Choose a reason for hiding this comment

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

Haven't reviewed all of it, but it looks good overall

@BrayanDSO BrayanDSO added Needs Author Reply Waiting for a reply from the original author and removed Needs Review labels Feb 7, 2025
@criticalAY criticalAY force-pushed the frag-migrate-myaccount branch 3 times, most recently from f66bf81 to 3747726 Compare February 9, 2025 19:20
@criticalAY criticalAY added the Blocked by dependency Currently blocked by some other dependent / related change label Feb 9, 2025
@criticalAY criticalAY force-pushed the frag-migrate-myaccount branch 5 times, most recently from a14930a to 4e1bfec Compare October 6, 2025 18:20
Copy link
Member

@BrayanDSO BrayanDSO left a comment

Choose a reason for hiding this comment

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

In the future, we ideally should rely less on AccountActivity and be able to launch the fragment anywhere.


override fun onConfigurationChanged(newConfig: Configuration) {
super.onConfigurationChanged(newConfig)
loggedInLogo.isVisible = !(isScreenSmall && newConfig.orientation == Configuration.ORIENTATION_LANDSCAPE)
Copy link
Member

Choose a reason for hiding this comment

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

If the parent activity doesn't have android:configChanges="orientation", this doesn't work.

I get that having a separate layout file just for that is a lot of boilerplate, so I'm fine with the current approach.

But do note at the class docs that the parent activity needs to listen to "orientation" configuration changes to work properly, and note why a separate layout file isn't being used.

@BrayanDSO BrayanDSO added the Needs Author Reply Waiting for a reply from the original author label Oct 29, 2025
@criticalAY criticalAY force-pushed the frag-migrate-myaccount branch from 4e1bfec to a9daf73 Compare October 29, 2025 11:14
- LoggedIn viewmodel to handle logged in state
- enum class LoginError to handle the errors related to Login and LoginState class for various login states
update: MyAccount Unit Tests

add activity to the activity list
- update usage to new LoginFragment

fix: multimedia uri security exception
@criticalAY criticalAY force-pushed the frag-migrate-myaccount branch from a9daf73 to c31dc7c Compare October 29, 2025 11:20
@criticalAY criticalAY removed Needs Author Reply Waiting for a reply from the original author Has Conflicts labels Oct 29, 2025
@BrayanDSO BrayanDSO added this pull request to the merge queue Oct 31, 2025
@BrayanDSO BrayanDSO added Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) and removed Needs Second Approval Has one approval, one more approval to merge labels Oct 31, 2025
Merged via the queue into ankidroid:main with commit d8aa781 Oct 31, 2025
10 checks passed
@github-actions github-actions bot added this to the 2.23 release milestone Oct 31, 2025
@github-actions github-actions bot removed the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Oct 31, 2025
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.

4 participants