sub-detail fixes #2303
sub-detail fixes #2303
Conversation
| @@ -73,11 +73,9 @@ public void setRoot(ViewGroup root) { | |||
|
|
|||
| mMenu = root.findViewById(R.id.tabbed_detail_menu); | |||
| mViewPager = root.findViewById(R.id.tabbed_detail_pager); | |||
| mViewPager.setId(AndroidUtil.generateViewId()); | |||
There was a problem hiding this comment.
this already has an id set in xml and setting dynamic ids result in fragment not getting re-attached to the activity.
There was a problem hiding this comment.
Good catch. It seems really strange that the original line was set at all, do you have a sense of what was trying to be accomplished with it? The only reason I can think you'd set the ID manually would be if you intended to have two ViewPagers within the same Activity.
I went back to check on the git blames and it does look like that is explicitly why that change was made:
d2eab7c
The only context where I can think this occurs is with persistent case tiles which do have their own view pager. The previous commit from the one I linked above is
which does discuss persistent case tiles, and it's in the persistent case tile PR
#81
I think the key thing to test is loading a case list screen with a view pager, along with a persistent case tile that has its own view pager. I think both end up in the same general view hierarchy, so they need distinct ID's just based on how the views work but it's possible that other refactors have made that unecessary.
| mViewPageTabStrip = root.findViewById(R.id.pager_tab_strip); | ||
|
|
||
| mViewPager.setOnPageChangeListener(new ViewPager.OnPageChangeListener() { | ||
| mViewPager.addOnPageChangeListener(new ViewPager.OnPageChangeListener() { |
There was a problem hiding this comment.
no-op change since setOnPageChangeListener is deprecated.
| @@ -73,11 +73,9 @@ public void setRoot(ViewGroup root) { | |||
|
|
|||
| mMenu = root.findViewById(R.id.tabbed_detail_menu); | |||
| mViewPager = root.findViewById(R.id.tabbed_detail_pager); | |||
| mViewPager.setId(AndroidUtil.generateViewId()); | |||
There was a problem hiding this comment.
Good catch. It seems really strange that the original line was set at all, do you have a sense of what was trying to be accomplished with it? The only reason I can think you'd set the ID manually would be if you intended to have two ViewPagers within the same Activity.
I went back to check on the git blames and it does look like that is explicitly why that change was made:
d2eab7c
The only context where I can think this occurs is with persistent case tiles which do have their own view pager. The previous commit from the one I linked above is
which does discuss persistent case tiles, and it's in the persistent case tile PR
#81
I think the key thing to test is loading a case list screen with a view pager, along with a persistent case tile that has its own view pager. I think both end up in the same general view hierarchy, so they need distinct ID's just based on how the views work but it's possible that other refactors have made that unecessary.
|
@ctsims thanks for the context. I tested that this PR does break the persistent case tiles when present along with a viewpager. I am going to investigate if there is a better way to handle these both together. |
|
Is there a regression test somewhere that would have caught this change in QA? If not, we should make sure to add one, given how close we were to missing this. I was very close to not catching the issue and only remembered what was going on because it seemed really strange that we would have been doing the original behavior. |
|
@ctsims took another look at it and it doesn't seem possible today for a single screen to contain both the viewpagers. the reason being -
|
|
@ctsims bumping this up for the repro of the edge case this PR breaks. |
|
Hi Shubham, I offlined with a replication for the state I was describing, as seen below |
|
@ctsims thanks I can repro the issue with this. I will followup with some test on it either with QA or Espresso. |
|
Can one of the admins verify this patch? |
EntitySubnodeDetailFragmentto avoid reloading data on configuration changesEntityLoaderTaskcompletes resulting in thedeliverLoadResultto operate on a changed session scope. The PR just discards the result of theEntityLoaderTaskif fragment is not visible.