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

WIP: added autohiding statusbar with scrollView.SetOnScrollChangeListener() #736

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

Conversation

bezmi
Copy link

@bezmi bezmi commented Sep 29, 2018

This addresses #114. I have tried to add the auto-hide functionality to the action bar by using scrollView.setOnScrollChangeListener(). I tried to do it with the webViewContent, but due to its nesting within the scrollview, it was really buggy.

The only issue I can see is that when the bar is showing/hiding, there is artificating at the bottom of the screen.

The system status bar doesn't get hidden, but I believe that would be relatively simple to implement.

On a side note, when playing around with the fling gestures to switch articles, I found that they were pretty unreliable. Changing them to apply on the scrollView seemed to make them much more reliable but this isn't something I've tested or fully understand...I am an android development newbie.

@bezmi bezmi changed the title added autohiding statusbar with scrollView.SetOnScrollChangeListener() Fixes #114 added autohiding statusbar with scrollView.SetOnScrollChangeListener() Sep 29, 2018
@bezmi
Copy link
Author

bezmi commented Sep 29, 2018

Checks are failing because the method I've implemented requires API level 23 and minimum is set to 14. To fix the flickering I think it would require the action bar to be overlayed on the content.

@Strubbl
Copy link
Contributor

Strubbl commented Sep 29, 2018

So what can we do about the API level problem? I would not like to merge this PR until travis is happy.

API level 14 means Android 4.0. API Level 23 would mean Android 6.0. A quick check of the stats of the installations of wallabag in Gplay reveals that about 15% of all users are using an API level below 23. This means 85% use Android API level 23 and above anyways.

From my point of view i would just increase the minimum API level. But what do the other developers think about it? //CC @wallabag/android

@bezmi
Copy link
Author

bezmi commented Sep 29, 2018

Can anyone provide advice on how to allow the Toolbar to overlay the content so that there is no flickering when scrolling? I have tried adding requestWindowFeature(Window.FEATURE_ACTION_BAR_OVERLAY) in the ReadArticleActivity class' onCreate method, however, this doesn't seem to change anything.

EDIT: I think it is solved. I added: <item name="windowActionBarOverlay">true</item> to styles.xml, which draws the action bar as an overlay, then added padding equal to the size of the action bar to the webview. No more flicker. I will push changes after looking into implementing autohide with backward compatibility to API level 14.

@bezmi bezmi changed the title added autohiding statusbar with scrollView.SetOnScrollChangeListener() WIP: added autohiding statusbar with scrollView.SetOnScrollChangeListener() Sep 30, 2018
@bezmi
Copy link
Author

bezmi commented Sep 30, 2018

@Strubbl I believe it is now ready for review. I have tested this code with the theme fixes in #730 and it fixes the action bar colour. Seems I was mistaken. I was incorrectly grabbing properties before the activity was fully loaded, which set the actionbar colour to the default. It is fixed now.

Do you think we still need #678 after this is merged?

Overview of changes:

  1. Added option to autohide system and app bar on scroll (default = on)
  2. Implemented hiding of system and app bar on tap in center of screen
  3. Added extra padding to the header in the CSS so the system and app bar do not overlay the displayed text.

added goFullscreen method to more easily toggle fullscreen on and off, made both status bar and action bar hide/show on scroll and if tapping center of screen

added layout_marginTop above the paddingTop to give 2x actionbar padding from top edge so that title does not get hidden behind status bar + action bar. Added fullscreen layout flags to onCreate of the article class so that view does not shift up when full screen is toggled

wrapped autohide actionbar on-scroll functionality in an if statement so it only works if API level is >= 23, otherwise it won't autohide. Implemented fullscreen on tap if the central 40% of screen is tapped.

adjusted tolerance of autohide to make it feel more snappy

removed unneccesary <item> tag for action bar overlay

removed padding from article.xml and added padding to CSS files.

added settings option for auto fullscreen

fixed actionbar colour issue - was getting decorview before the activity was loaded. Removed the actionBar.hide() and show() calls, un-neccessary with the decorView stuff
@bezmi bezmi force-pushed the auto_hide_actionbar branch from 27d4fdf to 6f093e5 Compare October 1, 2018 07:31
@cheywood
Copy link
Contributor

cheywood commented Sep 6, 2021

Hi @di72nn and all, it would be great to get something like this merged, providing a fullscreen reading experience without losing the ability to easily bring up the UI. Was there feedback on this PR and why it wasn't merged? I gave it a quick spin just now and it seems quite good.

Or are there other ideas about what is desired for fullscreen on scroll? #114.

@cheywood
Copy link
Contributor

Coming in for a re-nudge (sorry!). I'm guessing there was discussion somewhere about this not getting merged but I haven't been able to find that. @di72nn @Strubbl ?

@bezmi if this is believed ready to merge should WIP be removed?

@di72nn
Copy link
Member

di72nn commented Jan 18, 2022

Sorry, I still can't find the time to properly look into it.

I was against approaches like that, because I think it should be implemented using layouts. But seeing that no progress in the layout department has been made, approaches like the one in this PR may be acceptable. I haven't checked the PR though.

@cheywood
Copy link
Contributor

Thanks for following up. That's great, that explains the logic behind it not moving along at least.

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