852595 - Increase vertical space to show more remote tabs in tabs tray

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Lucas Rocha (:lucasr)]

Attachment: Tabs tray needs more space

Right now we only show two rows (see screenshot).

Comment 1

[Mark Capella [:capella]]

Attachment: bug852595v0.diff

This helps :)

Comment 2

[Mark Capella [:capella]]

Attachment: bug852595p2.diff Smooth mHeader display transition

The previous patch works, but mHeader re-animates from the top during the new re-sizing that accompanies switching to/from the sync tab (performing LayoutChanges ...)

It was mildly annoying visually, so I came up with this.

Comment 3

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8424414 [details] [diff] [review]
bug852595p2.diff Smooth mHeader display transition

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

Can you describe exactly what this does on device?  I'll try it tomorrow, but what should I be looking for?

::: mobile/android/base/tabspanel/TabsPanel.java
@@ +406,5 @@
>          final int toolbarHeight = resources.getDimensionPixelSize(R.dimen.browser_toolbar_height);
>          final int tabsPanelWidth = getWidth();
>  
>          if (mVisible) {
>              if (mIsSideBar) {

All these ifs blocks have the same condition, so they should be co-alesced.

@@ +441,2 @@
>  
> +                final int translationY = (mVisible ? 0 : -toolbarHeight);

nit: why re-organize these lines?

Comment 4

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #3)
> Comment on attachment 8424414 [details] [diff] [review]
> bug852595p2.diff Smooth mHeader display transition
> 
> Review of attachment 8424414 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you describe exactly what this does on device?  I'll try it tomorrow,
> but what should I be looking for?

Actually, I see it in https://bugzilla.mozilla.org/show_bug.cgi?id=852595#c2.

Comment 5

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8424366 [details] [diff] [review]
bug852595v0.diff

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

::: mobile/android/base/tabspanel/TabsPanel.java
@@ +140,5 @@
>      @Override
>      public void onTabChanged(int index) {
> +        // Screen resize isn't required by default on tab change, but it's possible on
> +        // a tablet when in portrait mode, and we're changing to/from REMOTE_TABS.
> +        boolean isTabletInPortrait = HardwareUtils.isTablet() &&

Can you explain why this is the only case?  It seems like needing resize shouldn't be hardware specific.

@@ +147,2 @@
>          if (index == 0)
> +            show(Panel.NORMAL_TABS, (isTabletInPortrait && mCurrentPanel == Panel.REMOTE_TABS));

Lift this boolean out of here.  Better yet, why are we keeping show(..., false) at all?  I see no consumers other than these; and it looks like show(..., true) is always correct.

If we do keep it, explain how this is different than |shouldExpand|; and explain why we wouldn't ask the panel to determine if it wants resize with a sibling method like |shouldResize|.

Comment 6

[Mark Capella [:capella]]

Attachment: bug852595.diff

I had tried to remove the show(panel, false) method in the first iteration, but wound up with that extra animation on every tab change. The (p0) patch fixed it but left the animation on the sync tab change.

After I spent some more time and removed the extra animation from the sync tab in the (p2) patch, I never thought to just go back, combine them, and wind up with this much simpler patch overall :-)

fyi - I would have coalesced that bit of animation code but one of my reviewers is sensitive to my cleaning up opportunities like that vs. isolating them into a separate "refactor"-specific patch.

I wasn't sure which camp you'd fall into ... so I just left it alone ;)

Comment 7

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8425246 [details] [diff] [review]
bug852595.diff

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

This is looking pretty great on my devices.  Huge improvement, and totally agree on making the header animation one-shot.  I'm going to run it by antlam for UX approval, so let's wait for that before landing, but r+ for the implementation (modulo one nit).

::: mobile/android/base/tabspanel/TabsPanel.java
@@ +393,5 @@
>          final Resources resources = getContext().getResources();
>          final int toolbarHeight = resources.getDimensionPixelSize(R.dimen.browser_toolbar_height);
>          final int tabsPanelWidth = getWidth();
>  
> +        if (mVisible && mIsSideBar) {

I'm finding these if blocks quite difficult to parse.  Can we make the structure:

if (mIsSideBar) {
  if (mVisible) {
  } else {
  }
} else {
  if (!mHeaderIsVisible) {
  }
}

Comment 10

[Mark Capella [:capella]]

I'll look to be sure, but I think that's slightly wrong

if (mIsSideBar) {
  if (mVisible) {
  } else { // Do Something
  }
} else {
  if (!mHeaderIsVisible) {
  }
}

 vs.

if (mIsSideBar) {
  if (mVisible) {
  }
  // then Do Something
} else {
  if (!mHeaderIsVisible) {
  }
}

Comment 11

[Nick Alexander :nalexander [he/him]]

(In reply to Mark Capella [:capella] from comment #10)
> I'll look to be sure, but I think that's slightly wrong

Yes, but you added the do somethings.  It's the big structure that's hard to parse, with the final else having a condition that you've handled earlier incorporated.

Comment 12

[Mark Capella [:capella]]

Attachment: bug852595.diff

(confused) ... This is what I'd be looking at next ...

Comment 13

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8425802 [details] [diff] [review]
bug852595.diff

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

::: mobile/android/base/tabspanel/TabsPanel.java
@@ +405,3 @@
>  
> +        } else if (!mHeaderVisible) {
> +            final Resources resources = getContext().getResources();

Yeah, pretty much.  Slight preference for not having else if, and instead having a nested if, but I find this much easier to parse than what you had earlier.

Comment 14

[Mark Capella [:capella]]

Ah! Now I understand :)

I used to style that way also, this particular format was ingrained on me by the (same) reviewer I mentioned earlier ... hehe ... I can do that if you'd like  :-D

Comment 15

[Nick Alexander :nalexander [he/him]]

capella: okay, this got antlam's approval.  And mine!  The resizing when the device rotates "pops" quite visibly, but I think that was the case before this patch too :(

Comment 16

[Mark Capella [:capella]]

Nice :) 

(Carrying over the r+ to the latest patch version)
Try push ... https://tbpl.mozilla.org/?tree=Try&rev=41580880d1b7

Comment 17

[Mark Capella [:capella]]

https://hg.mozilla.org/integration/fx-team/rev/ad81b123c0d8

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ad81b123c0d8