Closed Bug 878145 Opened 11 years ago Closed 3 years ago

Follow-up: auto-scroll to datareporting section when settings launched from notification

Categories

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED INCOMPLETE

People

(Reporter: liuche, Assigned: alex, Mentored)

References

Details

(Whiteboard: [good next bug][lang=java])

Attachments

(2 files)

I've merged the datareporting preferences into another screen, so when the Android system data notification is clicked, we want to scroll to the datareporting preferences.
I'm not actually sure this is possible, but if someone wants to investigate, I can mentor.
Assignee: liuche → nobody
Mentor: liuche
Whiteboard: [good next
I'm not actually sure this is possible, but if someone wants to investigate, I can mentor.
Whiteboard: [good next → [good next bug][lang=java]
I'd like to work on this as my second bug. I already did some research since Sunday and finally have found a promising approach.
I'll upload a patch later today or during the next days.
Great! Thanks for taking this bug Alex - flag me for review when you've got a patch up and I'll assign this to you. Thanks!
This is a first revision of the patch to show the idea of my solution and to get some feedback.

Besides the logic part I'm sure there are at least some things that could be renamed or split up etc. so please let me know of them ;).

These are the (simplified) steps:
- First I check if the vendor settings screen has been opened via the data reporting notification.
- If this is the case, I create a new thread to be able to use Thread.sleep() outside the UI thread.
- Now I want to get access to the ListView that contains the Preferences.
- Because the ListView is not yet available when onCreate() or any other of those functions is called, I first wait 500ms before I try to retrieve the ListView.
- If one of the required Views is not available, I wait another 500ms before I try it again. This repeats until all necessary Views are available.
- When the ListView can be accessed, the code determines the index of the "Data Reporting" headline within the ListView by iterating over the ListViews children and searching for the Preferences key attribute.
- Once the index is determined, I can use the ListViews smoothScrollByOffset() function.

Probably those loops and sleeps are things that should be avoided in general, but according to my research there seems to be no simple approach.
The two TODOs mark a possible synchronization problem. For me it worked fine all the time, but probably this is not the best possible solution.

Ok, now I'm really interested in some feedback ;).
Attachment #8605602 - Flags: feedback?(liuche)
Comment on attachment 8605602 [details] [diff] [review]
bug-878145-v1.patch

Review of attachment 8605602 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Alexander, thanks for picking up this bug! I had mentally filed this under "never going to be fixed" :P

I didn't know about the smoothScroll* methods, but that looks like exactly what we need. Since we support API 9, you'll probably want to use smoothScrollToPosition, which was added in API 8 (if that's deprecated in API 11+, we can also do a version check).

I looked through your patch, and I think there are definitely better alternatives to the thread sleeping approach. Android has a bunch of useful callbacks for when views are attached, or hierarchies change, so we can use those.

However, we actually iterate through all the preferences already, and I think they're already initialized. Take a look at this check, and see if you can add your scrolling code there!
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/preferences/GeckoPreferences.java#672

If that works out, flag me for feedback/review with your new patch!
Attachment #8605602 - Flags: feedback?(liuche)
Assignee: nobody → alex
(In reply to Chenxia Liu [:liuche] from comment #6)
> Since we support API 9, you'll probably want to use
> smoothScrollToPosition, which was added in API 8 (if that's deprecated in
> API 11+, we can also do a version check).
The method smoothScrollToPosition() is not deprecated, but it scrolls the selected element to the bottom and not to the top of the visible area. In my opinion this wired scroll position would be worse than not scrolling at all. I have found another replacement:

The line
  listView.smoothScrollByOffset(i);
can be replaced by
  listView.smoothScrollBy(listView.getChildAt(i).getTop(), 1000);
which is slightly more complex, but does exactly the same.

Should I use a version check for API level 11+ or use the same code for all versions?

> I looked through your patch, and I think there are definitely better
> alternatives to the thread sleeping approach. Android has a bunch of useful
> callbacks for when views are attached, or hierarchies change, so we can use
> those.
> 
> However, we actually iterate through all the preferences already, and I
> think they're already initialized. Take a look at this check, and see if you
> can add your scrolling code there!
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/
> preferences/GeckoPreferences.java#672
I'm able to determine the index with pref.getOrder() within setupPreferences(), but I'm not able to access the ListView. I need the ListView to trigger one of the smoothScroll* methods.
There is a method called getListView() but the returned ListView is always empty. Do you have an idea on how to access the ListView? I have invested some hours but I couldn't find something useful.
Flags: needinfo?(liuche)
OS: Mac OS X → Android
Hardware: x86 → All
Sorry for the delay, Alex! I've been at a conference, so I haven't had a chance to look at this until now.

> The method smoothScrollToPosition() is not deprecated, but it scrolls the
> selected element to the bottom and not to the top of the visible area. In my
> opinion this wired scroll position would be worse than not scrolling at all.
> I have found another replacement:
> 
> The line
>   listView.smoothScrollByOffset(i);
> can be replaced by
>   listView.smoothScrollBy(listView.getChildAt(i).getTop(), 1000);
> which is slightly more complex, but does exactly the same.
> 

You can use the same code for all of this, but let's add a comment on why this code has a constant offset. Since this value is in pixels, this will vary the height by device, we should try to use something that uses something from values.xml. I wonder if there's a listItem height there?

> 
> > I looked through your patch, and I think there are definitely better
> > alternatives to the thread sleeping approach. Android has a bunch of useful
> > callbacks for when views are attached, or hierarchies change, so we can use
> > those.
> > 
> > However, we actually iterate through all the preferences already, and I
> > think they're already initialized. Take a look at this check, and see if you
> > can add your scrolling code there!
> > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/
> > preferences/GeckoPreferences.java#672
> I'm able to determine the index with pref.getOrder() within
> setupPreferences(), but I'm not able to access the ListView. I need the
> ListView to trigger one of the smoothScroll* methods.
> There is a method called getListView() but the returned ListView is always
> empty. Do you have an idea on how to access the ListView? I have invested
> some hours but I couldn't find something useful.

Thanks for investigating! I'll take a look as well and get back to you.
Flags: needinfo?(liuche)
(In reply to Chenxia Liu [:liuche] from comment #8)
> Sorry for the delay, Alex! I've been at a conference, so I haven't had a
> chance to look at this until now.

No problem, I'm also short of time at the moment :).

> > The line
> >   listView.smoothScrollByOffset(i);
> > can be replaced by
> >   listView.smoothScrollBy(listView.getChildAt(i).getTop(), 1000);
> > which is slightly more complex, but does exactly the same.
> > 
> 
> You can use the same code for all of this, but let's add a comment on why
> this code has a constant offset. Since this value is in pixels, this will
> vary the height by device, we should try to use something that uses
> something from values.xml. I wonder if there's a listItem height there?

The second parameter here (1000) is the duration of the scroll animation in milliseconds [1]. I do not know of a standard or default value, so I used this fixed value which was used in an example.

The position is dynamically calculated by using "listView.getChildAt(i).getTop()". List items probably do not have a fixed height that could be used here (the heights also depend on the screen size, device orientation, language and so on).

[1] https://developer.android.com/reference/android/widget/AbsListView.html#smoothScrollBy%28int,%20int%29
(In reply to Chenxia Liu [:liuche] from comment #8)
> > > I looked through your patch, and I think there are definitely better
> > > alternatives to the thread sleeping approach. Android has a bunch of useful
> > > callbacks for when views are attached, or hierarchies change, so we can use
> > > those.
> > > 
> > > However, we actually iterate through all the preferences already, and I
> > > think they're already initialized. Take a look at this check, and see if you
> > > can add your scrolling code there!
> > > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/
> > > preferences/GeckoPreferences.java#672
> > I'm able to determine the index with pref.getOrder() within
> > setupPreferences(), but I'm not able to access the ListView. I need the
> > ListView to trigger one of the smoothScroll* methods.
> > There is a method called getListView() but the returned ListView is always
> > empty. Do you have an idea on how to access the ListView? I have invested
> > some hours but I couldn't find something useful.
> 
> Thanks for investigating! I'll take a look as well and get back to you.

Hi Chenxia, do you have any updates on this? Don't worry if not, just want to know the current state :).
Status: NEW → ASSIGNED
Flags: needinfo?(liuche)
Ahh, sorry about this, Alexander! This definitely fell through the cracks for me - I need to wrap some things up this week, but I will definitely take another look next week.

Keeping this needinfo so I don't forget!
Flags: needinfo?(liuche)
We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → INCOMPLETE
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.