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

make main page tabs scrollable and hide when there is only a single tab #2871

Merged
merged 5 commits into from Jan 2, 2020

Conversation

atpamat
Copy link
Contributor

@atpamat atpamat commented Dec 15, 2019

This PR makes tabs scrollable so they don't get squeezed if there are too many of them and hides tab selector if there is only a single tab. Adds and closes #1647

Before After
single tab before single tab after
multiple tabs before multiple tabs after

Created separate ScrollableTabLayout class to avoid cluttering MainFragment with switching between scrollable and fixed modes.

@TobiGr TobiGr added bug Issue is related to a bug GUI Issue is related to the graphical user interface labels Dec 15, 2019
@TobiGr TobiGr added this to the 0.18.1 milestone Dec 15, 2019
Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Thank you for this needed feature! :-D
The layout width calculation can be improved, see below ;-)

Comment on lines 63 to 127
private void resetMode() {
if (getTabCount() < 2) {
setVisibility(View.GONE);
return;
} else {
setVisibility(View.VISIBLE);
}

if (getTabCount() == 0 || getTabAt(0).view == null) return;
setTabMode(TabLayout.MODE_FIXED);

int layoutWidth = getWidth();
int minimumWidth = ((View) getTabAt(0).view).getMinimumWidth();
if (minimumWidth * getTabCount() > layoutWidth) {
setTabMode(TabLayout.MODE_SCROLLABLE);
return;
}

int actualWidth = 0;
for (int i = 0; i < getTabCount(); ++i) {
if (getTabAt(i).view == null) return;
actualWidth += ((View) getTabAt(i).view).getWidth();
if (actualWidth > layoutWidth) {
setTabMode(TabLayout.MODE_SCROLLABLE);
return;
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Could you add some comments to this function? For example explaining why getTabAt(0).view could be null and why you check for width twice (I assume you first do a fast check to improve performance?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

getTabAt() has a @Nullable annotation. I'll make this function a little cleaner and account for possible null tabs differently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nevermind, it returns null if index exceeds getTabCount().

There were no checks for whether Tab.view is null in some Tab's public methods modifying view so I think I can assume that it's not null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a few comments throughout the class and simplified the calculation of tabs' width. I also fixed my mistake where width was calculated based only on minimum width requested by tab even if it's contents were wider.

@atpamat
Copy link
Contributor Author

atpamat commented Dec 15, 2019

Version 1.1.0-alpha6 of material components adds MODE_AUTO to TabLayout.mode so once there is a release version and NewPipe migrates to it most of the functionality of ScrollableTabLayout will be covered by TabLayout (except for auto hiding when there are less than two tabs).

@TobiGr
Copy link
Member

TobiGr commented Dec 31, 2019

The changes look reasonable to me. @Stypox You began to review this. Can you please have a final look?

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Thanks, the ScrollableTabLayout class is now ok! I only pointed out a little inconsistency :-)

@atpamat atpamat requested a review from Stypox January 1, 2020 13:28
Stypox
Stypox previously approved these changes Jan 2, 2020
Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Thanks, all is good now :-D

@TobiGr TobiGr merged commit c56fb8c into TeamNewPipe:dev Jan 2, 2020
This was referenced Jan 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is related to a bug GUI Issue is related to the graphical user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

One Item No Tabs in v0.14.0
3 participants