Multiple tabs are opening on clicking the concept cards link thrice

[KolliAnitha]

Describe the bug Multiple tabs are opening on clicking the concept cards link thrice

To Reproduce Steps to reproduce the behavior:

1. Go to 'Percentages' - Revision
2. Click on 'Converting Percentages to Decimals'
3. Scroll down to click on equicalent fraction link multiple times
4. See the no.of tabs opened

Expected behavior Since the same link is clicked multiple times, single tab must be opened instead of multiple tabs

Demonstration https://user-images.githubusercontent.com/101634267/186579350-9d2f05b5-d408-40f0-aaa2-2afeb82b0d4b.mp4

Environment Device name: One plus Nord2 5G Android version : Android 11 App version : 0.8-alpha-091b45a188

[seanlip]

@BenHenning Just noting this since you might want to take a look if you're looking at concept cards currently.

Feel free to deassign if you don't think it's a quick fix.

[BenHenning]

This is a known problem, though I don't think there's an issue filed for it yet.

The work I'm doing with concept cards is mostly unrelated to this. I think the fix is to not open the concept card dialog if it's already open, but that's a bit tricky in the new world where multiple concept cards can be opened at once (i.e. for cases when a concept card links to another). We will have to look into introducing dynamic keys to make that work properly, based on the skill ID being shown.

This is probably not a very long fix, but it's certainly not a quick one.

[rishujam]

Hey, I am new to this project. I can't find the percentage revision

[BenHenning]

Hi @rishujam, welcome!

The Percentages lessons are production-only, so they aren't available in the local developer build of the app. That being said, you could try to repro this issue by using concept cards that are available in the local app. Concept cards can be accessed via the revision cards in the "first test topic" topic that you can open from the local developer build of the app.

You could also download and install the beta version of the app from Play Store to get access to the Percentages lessons (at least to try and build familiarity with how to repro the issue as that understanding may be transferable over to the developer version that you can iterate on).

[rishujam]

?

[seanlip]

Hi Subhanshu, can you give a brief description of how you are tackling this? What files have you changed, what is the actual cause of the issue, do you have a screenshot or video showing that your fix works?

Feel free to post any of the above here and if it looks like it is in the right direction, we can assign this to you. Thanks!

Message ID: @.***>

[rishujam]

This is the result after multiple clicks it only opens once.

https://user-images.githubusercontent.com/74773876/201156398-6697e0c9-c583-444d-9b1f-04b346806562.mp4

Approach: I made a state class for the card (open, close, or default) and after the first click, I set that state to opened and checks the state on the click listener before opening the dialog.

[seanlip]

Thanks! One question -- will your fix work in the case where a concept card contains an interior link to a different concept card?

[rishujam]

Yes, but I have to write this state code there also (everywhere this issue is occurring). If you could list where are these occurring I can fix those all.

[seanlip]

Actually, any concept card can have a link to a different concept card. So you'd need to write a fix to handle this case such that it would automatically work anywhere concept cards are used (including new additions to the codebase later). I think this is why Ben said it's a bit tricky to fix this.

If you have an idea how to do this, that's great, but if not, you might want to tackle a different issue instead since the proposed solution might not be maintainable.

[seanlip]

Yes, I think so -- @BenHenning may be able to provide more colour here, but I think that that is the basic idea.

Though, thinking about it, this gets a bit complex -- what if concept card A references B, which in turn references A again? So some product design might be needed here. To start with, I think we can just stack them: open each card "in front" of the previous one, and the user has to close it to reveal the previous card. It's fine to impose a "max depth" of 3 or 5 cards too, but that is optional. Will your approach account for this case?

[rishujam]

Not for now but will try.

[seanlip]

OK, thanks! Feel free to ping this thread with a video and description of your approach when you have an update.

[rishujam]

Hey, how about this? We can maintain a stack. See if the top of the stack is the same as the Card we are going to add then we do not open it. If it's different then only we open it.

[seanlip]

In the case where you don't open it, what is the experience like for the user?

On Thu, Nov 10, 2022, 11:41 PM Sudhanshu Kumar @.***> wrote:

Hey, how about this? We can maintain a stack. See if the top of the stack is the same as the Card we are going to add then we do not open it. If it's different then only we open it.

— Reply to this email directly, view it on GitHub https://github.com/oppia/oppia-android/issues/4532#issuecomment-1311342295, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACQV5SXDITZZ7XJK7XJ3W5LWHX2CNANCNFSM57RUV2LQ . You are receiving this because you commented.Message ID: @.***>

[rishujam]

The only case we do not open the card is those extra clicks done by mistake. Which is our goal to neglect right?

Example: A -> B are 2 Cards that need to be opened on top of each other. First card A opens (add card A to stack) and now to open card B user clicked 3 times. Now for the first click, B will be added to the stack and the card will open. The rest 2 clicks will be neglected as we will check the stack before opening another card. This way there will not be multiple cards B.

[seanlip]

Oh, I see. Yup that sounds fine! Thanks!

I'll assign you, feel free to make a PR.

[rishujam]

Need help What I thought: We need something that lives as long as all the cards in order to maintain the stack.

1. Parent fragment from where the first card was opened- Thought this way is not good for testing as we will need to write that code in every parent fragment from where cards are being created.
2. Can't add in ConceptCardFrag too as the different cards will have a different instance of ConceptCardFrag and different ViewModel inside them.

Where should I attach the stack?

[rishujam]

Implemented a solution. Will create PR once I go through PR guidance and code style.

[BenHenning]

Need help What I thought: We need something that lives as long as all the cards in order to maintain the stack.

1. Parent fragment from where the first card was opened- Thought this way is not good for testing as we will need to write that code in every parent fragment from where cards are being created.
2. Can't add in ConceptCardFrag too as the different cards will have a different instance of ConceptCardFrag and different ViewModel inside them.

Where should I attach the stack?

Hmm, concept card already stacks by default since it's a dialog fragment. We usually manage cross-activity state using domain-level controllers (that can live for the lifetime of the application), but that's tricky here since we don't have an easy way to associate backend state with showing/hiding dialog fragments.

I think your working approach in #4719 could be made to be a robust solution, but a few changes would be needed:

• We should avoid showing the concept card if it's already showing (rather than dismissing after the fact--this will avoid potential visual jank).
• We should make sure that we have a way of strictly observing the lifetime of the concept card (perhaps by binding to the fragment's lifecycle?) to ensure that the tracked list is always kept up-to-date.
• The utility holding the backstack needs to be injectable and marked as @Singleton scope so that it retained across fragment instances.

We may even be able to generalize this for all dialog fragments, but let's start with concept cards first and see what the solution ends up looking like @rishujam.

[rishujam]

I have to write a test file for the back stack manager and I just got into unit testing and roboelectric. I have some questions related to this.

1. As my main manager class has just contains 4 functions of add, remove, peek, destroy so I don't need to write test about that right?
2. I have to test that the lifecycle of the manager stick to concept cards until all the cards are closed.
3. If possible can you send any resource about lifecycle testing or any demo inside this project that do somethings similar.

[rishujam]

Please guide me about testing this back stack manager.

[BenHenning]

I have to write a test file for the back stack manager and I just got into unit testing and roboelectric. I have some questions related to this.

1. As my main manager class has just contains 4 functions of add, remove, peek, destroy so I don't need to write test about that right?
2. I have to test that the lifecycle of the manager stick to concept cards until all the cards are closed.
3. If possible can you send any resource about lifecycle testing or any demo inside this project that do somethings similar.

@rishujam per (1), each class that can be tested, should be tested. We have a few exceptions today (presenters, view models, and interfaces), but we try to test every other class. The reasons for this are two-fold: (a) Having narrow, specific tests allow us to catch regressions closer to the source of the failure (this is part of the "fail fast" philosophy. (b) Tests are part of public contract of a component. They more exactly define exactly what behaviors a component is capable of supporting (vs. just an API and documentation which don't actually strong enforce these behaviors).

Per (2), I'm not sure what the question is here--could you maybe clarify? Generally, we try to test all major "happy path" (i.e. cases where nothing goes wrong) and failure cases.

Per (3), I can't think of a close example at the moment, but to some extent all of our orientation change tests are verifying a lifecycle-related test. We also have LiveData tests in DataProviderTest, but those might be a bit complicated. The main idea behind testing lifecycles is determining the different possible states of an object within its lifecycle, the conditions that could lead to those states, and then creating tests to verify each of these scenarios.

[rishujam]

Thanks, last point (3) gave me some clarity.

[rishujam]

Hey @BenHenning, As you said we can follow the factory method to create the instance of ConceptCardFragment so here's what I have done.

1. Injected the stack manager in the factory, Factory manages the creation of card objects.
2. Now we need to inject the factory in every class where we need to create ConceptCard and that's what I did. Is this way fine?

[BenHenning]

Hey @BenHenning, As you said we can follow the factory method to create the instance of ConceptCardFragment so here's what I have done.

1. Injected the stack manager in the factory, Factory manages the creation of card objects.
2. Now we need to inject the factory in every class where we need to create ConceptCard and that's what I did. Is this way fine?

Your description sounds correct @rishujam but I'd need to see the code to be sure.