515648 - [faceted search] Make the size of Gloda searchbar customizable

Status: NEW

(Thunderbird :: Search, enhancement)

Description

[Christoph Grether]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2
Build Identifier: 

Since Bug 474711 has landed, I saw, that the new search box also contains the functionality of the old QuickSearch.

I always used the small QuickSearch in my toolbar along with a couple of other buttons. The new search box is too big, I simply have no more free space for my other buttons in the toolbar. 

Please add the ability to change the size of the Searchbox, like it's possible with the search bar in Firefox.

This would allow people to use the QuickSearch the same way, like they did in Thunderbird 2.0



Reproducible: Always

Comment 1

[rsx11m]

Confirming - it's a problem if users want to put additional buttons onto the toolbar, you are running (too) quickly out of vertical space.

Comment 2

[rsx11m]

Hmm, I just did some more testing. While you cannot explicitly change the size of the search bar, it is shrinking as you add further buttons to the toolbar from the "Customize" palette. Thus, my comment #1 may not be quite applicable.

Do you see the same?

Comment 3

[Bryan Clark (DevTools PM) [:clarkbw]]

Christoph, can you provide a screenshot of the problem?

The search box will resize to take available space.

Comment 4

[Christoph Grether]

Partially, it is shrinking down, but if you add for example a 'flexible space', the size won't automatically change like I want.

Plus users cannot resize to personal wishes.
I think the user and not the search bar should decide, how much space the search bar takes.

Even with that auto resize effect, it's not possible to restore the size of the old quick search for me. it simply lacks flexibility, so I think this Bug is valid.

Comment 5

[David Ascher (:davida)]

You can add many "space" and "flexible space" widgets to change the proportion of the empty space that it uses, and whether it scales or not.

Comment 6

[David Ascher (:davida)]

I've looked at how firefox does something similar, and it's certainly doable -- from what I can tell they automatically add splitter elements in between the two search bars.  

If someone wanted to tackle this, it should be a fairly simple XUL/JS project.  

(IIRC, Postbox does something related too, but I don't think we can see the relevant code)

Comment 7

[Christoph Grether]

Well, to be fair, I must agree, the auto resizing is working better than expected, but I still think this bug is valid.

David is right, adding spaces again and again could be used as a workaround to force, that the searchbar has the same size like the TB 2.0 QuickSearch had.

But it isn't really user friendly and the ability to resize the bar, like it is possible in Firefox would be really a nice polish thing.

If a user for example just uses a few buttons in his toolbar, the search eats up most of the space, unimportant, if the user want such an huge searchbar or not. Or if a user (like me) has more buttons in the toolbar and wants a searchbar, which has nearly the same size like the QuickSearch in TB 2 had, he must add a lot of 'Spaces', which is nothing more, than an annoying workaround.

Otherwise I'm sorry for bugspam.

Comment 8

[IagoSRL]

Hi!
I think that more width visually give more importance to the search bar, but What if we give more height? The search box looks more relevance (and with less vertical-lost-space; perhaps the largest letter -like new google page-). I add an attachment with screenshots to compare (old width, different heights, same letter size)

Any UI designer in the room?

// sorry for my bad english!

Comment 9

[IagoSRL]

Attachment: Search-box with differents height sizes

Comment 10

[Bryan Clark (DevTools PM) [:clarkbw]]

in bug 515769 we added some CSS changes to increase the search box size, though only on Windows.  I think Linux could use a similar 2px padding to give some room around the text and button.

Comment 11

[Bryan Clark (DevTools PM) [:clarkbw]]

also, I filed bug 516585 to look into Linux theme changes that could be made

Comment 12

[mikey.osd600]

Attachment: A new function, UpdateSpringSplitterState(), is added to the msgMail3PaneWindow.js and mailCore.js

This code was run on Vista so far. It has not ran on Mac or any other OS yet.

Comment 13

[Phil Ringnalda (:philor)]

Comment on attachment 408162 [details] [diff] [review]
A new function, UpdateSpringSplitterState(), is added to the msgMail3PaneWindow.js and mailCore.js 

Yeah, I'm pathetic enough that I can't even remember I need to look at this unless I request review from myself :)

Comment 14

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 408162 [details] [diff] [review]
A new function, UpdateSpringSplitterState(), is added to the msgMail3PaneWindow.js and mailCore.js 

wow, this seems to work really well for me.  Tried it out on the mac today and I'm not seeing any issues so far.

Resizing works well.  I added and removed the search bar manually, after clicking ( Done ) the splitter appeared and resizing worked.  The same applied with I removed the search bar and used ( Restore Default Set )

Comment 15

[Ronald Killmer]

One question that can be answered by testing placement of the search box into the menu bar. Does the new behavior work the same in both Tool and menu bars?

Comment 16

[mikey.osd600]

From the looks of it, it seems to work the same in the Tool and menu bars. 

However, there is one problem that I just noticed with my code: it doesn't remove the old toolbarsprings. If I move my existing searchbar to another location on the toolbar, the toolbarspring, that might have been added, will remain. This will cause huge gaps on the toolbar. I'll try fixing it by today.

Comment 17

[mikey.osd600]

Attachment: I removed and added some additional code to the UpdateSpringSplitterState() function to prevent duplimate splitters and springs.

In this patch, when the user clicks ( Done ) in the Customize Toolbar window, it will remove the existing searchbar's splitter and spring, and re-add them if there is a searchbar. This is my way from getting rid of duplicate splitters and springs that I used to have in my first patch.

Comment 18

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 408162 [details] [diff] [review]
A new function, UpdateSpringSplitterState(), is added to the msgMail3PaneWindow.js and mailCore.js 

out with the old, in with the new attachment 408547 [details] [diff] [review]

Comment 19

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 408547 [details] [diff] [review]
I removed and added some additional code to the UpdateSpringSplitterState() function to prevent duplimate splitters and springs.

adding philor to this one now

Comment 20

[Bryan Clark (DevTools PM) [:clarkbw]]

set some whiteboard status and (initial) target milestone

Comment 21

[Nomis101]

After applying this patch I now can minimize the searchfield and than move it to the left in the toolbar. Don't know if this is wanted.

Comment 22

[Phil Ringnalda (:philor)]

Comment on attachment 408547 [details] [diff] [review]
I removed and added some additional code to the UpdateSpringSplitterState() function to prevent duplimate splitters and springs.

Since I've utterly failed you, let's see how our shiny new toolbar submodule owner does instead.

Comment 23

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 408547 [details] [diff] [review]
I removed and added some additional code to the UpdateSpringSplitterState() function to prevent duplimate splitters and springs.

So, first off, this patch seems to have bitrotted, so we’re going to need a new one no matter what else I say.  (Feel free to blame philor for that.  That’s what I always do, anyways.  ;)

Next up, you seem to have a lot of ^Ms, and trailing spaces in the file when I look at it on my unix box.  Please remove them.

And now, on to the content of the patch itself…

>+++ b/mail/base/content/mailCore.js
>@@ -188,16 +191,56 @@ function MailToolboxCustomizeDone(aEvent
>+function UpdateSpringSearchSplitterState()

Why did you define this function in both mailCore.js and msgMail3PaneWindow.js?  The comment at the top of mailCore.js says:
/*
 * Core mail routines used by all of the major mail windows (address book,
 * 3-pane, compose and stand alone message window).  Routines to support custom
 * toolbars in mail windows, opening up a new window of a particular type all
 * live here.  Before adding to this file, ask yourself, is this a JS routine
 * that is going to be used by all of the main mail windows?
 */
So, it looks like you could reasonably add it to mailCore.js, and just use it from msgMail3PaneWindow.js.

And along those lines, is there a simple change you could make that would let it do the same thing for the search-container in the AddressBook window, cause that would be really cool.

>+{
>+  var searchbar = document.getElementById("gloda-search");
>+  var splitter = document.getElementById("gloda-search-splitter");
>+  var spring = document.getElementById("gloda-search-spring");
>+  var ibefore = null;

I think the standard these days is to use "let" instead of "var" there.  And, you could even move the "let ibefore" into the "if (seachbar) {" block below.  Although, now that I write that, I’m wondering why you have the ibefore variable at all, since it is only used in the if statements, and only once per assignment.

>+
>+  // to avoid having duplicate splitters and springs
>+  if(splitter)

You need a space before the ( here, and three more times below.

>+    splitter.parentNode.removeChild(splitter);
>+  if(spring)
>+    spring.parentNode.removeChild(spring);
>+
>+  if (searchbar) {
>+    
>+    ibefore = (searchbar.previousSibling).tagName;

I don’t think you need the ()s around searchbar.previousSibling here, or down below.

>+    
>+    // check if a splitter is before the searchbar
>+    if (ibefore != "splitter"){

We need a space before the { here, and down below.

>+      splitter = document.createElement("splitter");
>+      splitter.id = "gloda-search-splitter";
>+      splitter.setAttribute("resizebefore", "flex");
>+      splitter.setAttribute("resizeafter", "flex");
>+      splitter.className = "chromeclass-toolbar-additional";
>+      searchbar.parentNode.insertBefore(splitter, searchbar);
>+    }
>+
>+    ibefore = (splitter.previousSibling).tagName;
>+
>+    // check if a spring is before the splitter
>+    if(ibefore != "toolbarspring"){
>+      spring = document.createElement("toolbarspring");
>+      spring.id = "gloda-search-spring";
>+      spring.setAttribute("flex","1");

We need a space after the comma here.

>+      spring.className = "chromeclass-toolbar-additional";
>+      searchbar.parentNode.insertBefore(spring, splitter);
>+    }
>+  }
>+}

And some UI weirdness I’ve seen:
If I move the spacer from in front of the splitter to somewhere else on the toolbar, it moves itself back.  I understand why it’s doing that, but I think it might be nicer if you removed the id from that spacer instead of deleting it, and created a new one in front of the splitter.

As well, if I resize the search bar while I’m in customize more, that change doesn’t stick when I hit the "Done" button.

As one last point, we're asking developers to include tests for all new patches.  You first posted this patch before that policy came into effect, so I'm not going to block this patch if you don't provide any, but it would be really cool if you could add some.

Having said all that, I'm generally happy with the patch, and think that with the few tweaks/additions mentioned, it'll be good to go.

Thanks,
Blake.

Comment 24

[Nomis101]

Attachment: Unbitrottet above patch

Its a nice patch I sometimes use in my own builds, but in the meantime its bitrottet. So I've unbitrottet it to current trunk. Its the same patch as above, only unbitrottet (and removed trailing whitespaces).
I've tested the unbitrottet patch and it has the same affect to the searchbar as the original patch.

Comment 25

[Thomas D. (:thomas8)]

Nomis, have you addressed the tweaks and additions requested by Blake's review comment 23? If not, please do. If yes, please set the review? flag for your patch to bwinton(at)...

Comment 26

[Nomis101]

(In reply to comment #25)
> Nomis, have you addressed the tweaks and additions requested by Blake's review
> comment 23? 

No sorry, the "need a space before" thing is easy to do, but for the other things you need JS skills to address this. And I don't have. I can address as much things as I can and leave the rest of it for somebody with JS skills...