595224 - Tweak sizing algorithm in tab view to avoid empty space at bottom

Status: RESOLVED WONTFIX

(Firefox Graveyard :: Panorama, enhancement, P3)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

After sharing my screenshot (attached), discussion in #tabcandy went like this (with other text removed):

iangilman: zpao: if the algorithm's working as directed, then bumping up the tab size so 6 tabs fit neatly across would cause the tabs to be too big to all fit vertically
iangilman: zpao: so I agree it looks like a lot of wasted space, but I'm not sure what the answer is
...
iangilman: but yeah, maybe if the tabs were a little bigger, and we did 6 across, and the extra space was distributed between the tabs?
aza: iangilman: That is what I'm thinking.

Comment 1

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: screenshot

Comment 2

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v1

This is a first patch which needs some work.
- there are some weird interactions right now, by virtue of the two step logic. Not sure... Ian, how does it feel to you as you resize groups?
- the tabs must still need to be centered and keep a set margin on the right hand side. Right now the right margin (from the very right of the rightmost column to the edge of the group) can get adjusted.

Screenshot before this patch:
http://img.skitch.com/20100923-e6m5944mchpys9k228y7hngq2y.png

With this patch:
http://img.skitch.com/20100923-dj84pf4ynufw248x3h92jjd92s.png

Comment 3

[Ian Gilman [:iangilman]]

Comment on attachment 477811 [details] [diff] [review]
Patch v1

Seeing it in action, I have to say I really dislike that the padding grows and shrinks as you resize it. For that matter, I think having a number of groups on the screen each with different padding is going to look really ugly. 

Since variable padding seems like the only way to address this issue, I propose we don't fix it at all. 

I mean, I'm all for using the space efficiently, but really all we're trying to do here is look like we're using the space efficiently, and that's not worth making it ugly over.

Comment 4

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #3)
> Seeing it in action, I have to say I really dislike that the padding grows and
> shrinks as you resize it. For that matter, I think having a number of groups on
> the screen each with different padding is going to look really ugly. 
> 
> Since variable padding seems like the only way to address this issue, I propose
> we don't fix it at all. 

I agree. Marking this as wontfix.

That says, if someone has a brilliant patch which does this in a smoother, nicer fashion. For the time being, however, our resources are better spent elsewhere.

Comment 5

[Aza Raskin [:aza]]

The wontfix makes sense, at least for now. Need to do some thinking.

Comment 7

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #4)
> That says, if someone has a brilliant patch which does this in a smoother,
> nicer fashion.

This is not a full sentence. I meant "if someone has a brilliant patch..., we would consider it."

Comment 8

[Andreas Jung]

I thought I should simply accept that my bug (bug 626109) was duped to this wontfix bug, but on second thought I dare to disagree that this bug is a wontfix.

I think it should be reopened and simply set to Severity: enhancement.

The reasons for that are:
1) Everybody seems to agree that it would be nice if this was fixed and agrees that it actually is a problem. It feels wrong if it is resolved wontfix in this case.
2) If I was a developer with free time and looked around on what to work on, my motivation to work on a bug that is marked as wontfix would be very low if not non-existent. If it is an enhancement request (with low priority and target=future) it would be at least a bit more likely that I would invest time.
3) It's not like it would be the only bug report that has a status of new or reopened that suffers from the problem that nobody has the time or an idea of how to fix it properly.

That said I would try this patch (and depending on how complex the code is) make improvements or suggest some, but 1) I don't have a complete development environment installed at the moment and mitcho mentioned in bug 626109 that the patch doesn't apply anymore anyway. :(

Back to the topic, I don't even now if the approach I suggested in bug 626109 is the same as the approach in the patch attached here. Judging from the code alone (and the comments but without actually having a build with the patch) it seems to try to increase the thumbnails' size by adjusting the padding between them.

I had suggested that TabCandy should check if thumbnails are larger when ordered horizontally or vertically and use the case where they are larger.

Instead of:
┌--------------┐
|┌-----┐┌-----┐|
||  1  ||  2  ||
||     ||     ||
|└-----┘└-----┘|
|┌-----┐       |
||  3  |       |
||     |       |
|└-----┘       |
|              |
|              |
|              |
|              |
|              |
|              |
|              |
└--------------┘

Use this:
┌--------------┐
|┌-------┐     |
||       |     |
||   1   |     |
||       |     |
|└-------┘     |
|┌-------┐     |
||       |     |
||   2   |     |
||       |     |
|└-------┘     |
|┌-------┐     |
||       |     |
||   3   |     |
||       |     |
|└-------┘     |
└--------------┘

You could get some not-nice-looking empty space distribution if you use a lot of almost quadratic groups, but I think most people use rectangular groups, in which case it should look better.

To avoid flickering while resizing, the rearrangement of the thumbnails should not happen before the user finishes the resizing. (This would, by the way, also fix an existing edge case where resizing a group causes the thumbnails to flicker between normal and stacked view).

Something like:
┌--------┐   ┌--------┐   ┌--------┐
|┌--┐┌--┐|   |┌--┐┌--┐|   |┌----┐  |
||1 ||2 ||   ||1 ||2 ||   ||1   |  |
|└--┘└--┘|-->|└--┘└--┘|   ||    |  |
|┌--┐    |(1)|┌--┐    |   |└----┘  |
||3 |    |   ||3 |    |   |┌----┐  |
|└--┘    |   |└--┘    |-->||2   |  |
└--------┘   |        |(2)||    |  |
             |        |   |└----┘  |
             |        |   |┌----┐  |
             |        |   ||3   |  |
             |        |   ||    |  |
             |        |   |└----┘  |
             └--------┘   └--------┘
(1) resizing in progress / mouse button hold
(2) resizing finished / mouse button released

If you agree with me, please reopen the bug (I don't have the rights to do it :( ),
if you don't agree, please give me a convincing reason why it's better when this bug is wontfix, I was not able to find one.

Comment 9

[Michael Yoshitaka Erlewine [:mitcho]]

Andreas, I do think you hve a convincing argument, at least for the vertical orientation (and thank you for the pretty ASCII art!). I'm not sure we necessarily want to go down this road if we have mmore than one column in our layout (and thus have to adjust white space in between the columns) but your idea seems sound to me for the single-column (very vertical) layout.

I'll reopen this, though it probably won't get touched for fx4.

Comment 10

[Justin Dolske [:Dolske]]

Panorama has been removed from Firefox 45, currently in Beta and scheduled for release on March 7th. As such, I'm closing all existing Panorama bugs.

If you are still using Panorama, you will see a deprecation message in Firefox 44, and when 45 is released your tab group data will be migrated to bookmarks, with a folder for each group. There are also a few addons offering similar functionality.

See https://support.mozilla.org/en-US/kb/tab-groups-removal for more info.

We're removing Panorama because it has extremely low usage (about 0.01% of users), and has a large number of bugs and usability issues. The cost of fixing all those issues is far too high to justify, and so we'll instead be focusing our time and energy on improving other parts of Firefox.