Closed Bug 402177 Opened 17 years ago Closed 17 years ago

Add functionality for hiding the unifinder easily

Categories

(Calendar :: Calendar Frontend, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: chris.j.bugzilla, Assigned: sipaq)

References

Details

Attachments

(4 files, 10 obsolete files)

74.61 KB, application/zip
Details
112.12 KB, application/zip
Details
33.39 KB, patch
Fallen
: review+
sipaq
: ui-review+
Details | Diff | Splinter Review
3.86 KB, patch
Fallen
: review+
Details | Diff | Splinter Review
Currently the unifinder is always visible. For hiding it users need drag the splitter. This is not very intuitive.

It would be good to have a closer, a toolbar item, key board shortcut & a menu item for toggling the visibility of the unifinder.
Flags: wanted-calendar0.8+
OS: Windows XP → All
Hardware: PC → All
Summary: Add functionanlity for hiding the unifinder easily → Add functionality for hiding the unifinder easily
This patch takes care of hooking up the unifinder display into Sunbird's menubar.

I want to wait with a corresponding Lightning patch until bug 405303 (Thunderbird bug) is fixed, which would make this much easier on Lightning.

Please note that this patch also contains the patch for bug 306692 as it patches the same files and I don't enough disk space left for another source tree.
Assignee: nobody → bugzilla
Status: NEW → ASSIGNED
Attachment #290101 - Flags: ui-review?(christian.jansen)
Attachment #290101 - Flags: review?(michael.buettner)
Sorry for the delay, I'll do the ui review as soon I can build on Leopard....
Some general thoughts...
- The opened Unifinder should provide a closer
- It should be possible to call the Unifinder from the menu & the toolbar
Comment on attachment 290101 [details] [diff] [review]
Hook up unifinder display to Sunbird

We already put a lot of work into moving the unifinder to /base/content in order to share the code between Lightning & Sunbird. I don't see a reason for introducing code that doesn't make use of this. Lightning as well as Sunbird use the same xul content, see [1]. The unifinder itself is contained inside the 'bottom-events-box', see [2], the splitter 'calendar-view-splitter' divides the views and the unifinder, see [3]. We just need to hide/show these elements in order to hide/show the unifinder. All of this should be wired into the common command sets, see [4], which is also shared between both applications. Unfortunately, I need to minus this patch for now.

[1] http://lxr.mozilla.org/mozilla1.8/source/calendar/base/content/calendar-unifinder.xul
[2] http://lxr.mozilla.org/mozilla1.8/source/calendar/base/content/calendar-unifinder.xul#59
[3] http://lxr.mozilla.org/mozilla1.8/source/calendar/base/content/calendar-unifinder.xul#149
[4] http://lxr.mozilla.org/mozilla1.8/source/calendar/base/content/calendar-common-sets.xul
Attachment #290101 - Flags: review?(michael.buettner) → review-
(In reply to comment #2)
> - The opened Unifinder should provide a closer
> 
I guess such a closer would fit best right beside the search bar, but what if you then uncheck the search bar in Sunbird's view menu? As far as I can imagine the closer would either disappear too or be kept as the only visible element above the event list. The first possibility doesn't make sense, the second doesn't look good and wouldn't let you save space (but why else should you hide the search bar?).
Is there an elegant solution for providing a closer and keep the possibility for toggling the search bar?
In my opinion there is no need for an additional closer button.

The requested menu command and toolbar button should have a toggle functionality like show/hide the unifinder. In addition the splitter can be used to show/hide the unifinder.
This patch takes care of the review comment by mickey (comment 3) and moves 
the ability to show the unifinder to a shared location. I'm moving the review 
request to Philipp as Mickey is on vacation.

This patch still has a few minor issues:
1. It has no closer (I agree with Stefan in comment 5 that we need no closer 
   *if* we have a toolbar button that enables/disables the unifinder
2. It misses the toolbar button part from Christian (comment 2)
   Christian, if you could supply me with a toolbar button, I will gladly 
   add it to the patch.
3. It contains a small part of the patch from bug 389522 as it patches the 
   same file and I don't enough disk space left for another source tree.

Christian, I'd also be glad if you could state your thoughts again (regarding 
the open issues and the other parts of the UI-review (e.g. wording)). I will 
attach screenshots for this shortly.
Attachment #290101 - Attachment is obsolete: true
Attachment #293411 - Flags: ui-review?(christian.jansen)
Attachment #293411 - Flags: review?(philipp)
Attachment #290101 - Flags: ui-review?(christian.jansen)
Attached image Unifinder disabled (obsolete) —
Attached image Unifinder enabled (obsolete) —
The menu option should not be under View->Toolbars. The unifinder is not a toolbar. I would never look into that menu if I wanted to disable the list.
Same patch as attachment 293411 [details] [diff] [review]. I just changed the placement of the "event list" in the menubar after reading mvl's comment. Everything said in comment 6 still applies.
Attachment #293411 - Attachment is obsolete: true
Attachment #293417 - Flags: ui-review?(christian.jansen)
Attachment #293417 - Flags: review?(philipp)
Attachment #293411 - Flags: ui-review?(christian.jansen)
Attachment #293411 - Flags: review?(philipp)
Attached image Unifinder disabled - 2nd try (obsolete) —
Attachment #293412 - Attachment is obsolete: true
Attached image Unifinder enabled - 2nd try (obsolete) —
Attachment #293413 - Attachment is obsolete: true
(In reply to comment #6)
> Created an attachment (id=293411) [details]
> Hook up unifinder to Lightning & Sunbird
> 
> This patch takes care of the review comment by mickey (comment 3) and moves 
> the ability to show the unifinder to a shared location. I'm moving the review 
> request to Philipp as Mickey is on vacation.
> 
> This patch still has a few minor issues:
> 1. It has no closer (I agree with Stefan in comment 5 that we need no closer 
>    *if* we have a toolbar button that enables/disables the unifinder

I don't agree that a closer is not needed, but we can add it later. In long term I'd like to see the unifinder as a search result pane, which occurs automatically if users perform a search in the "not yet implemented" search box, located at the same position as in Thunderbird.


> 2. It misses the toolbar button part from Christian (comment 2)
>    Christian, if you could supply me with a toolbar button, I will gladly 
>    add it to the patch.

I'll create you a set of PNGs. Stay tuned. Please locate the "Unifinder" toolbar button between "Month" and "Delete".

Regarding the label. I'm not sure if "Event List" hits the nail on the head. From an users point of view the unifinder is more a search, or filter. Maybe we should call it just "Search Calendar"

... Month | Search Calendar | Delete ...

> 3. It contains a small part of the patch from bug 389522 as it patches the 
>    same file and I don't enough disk space left for another source tree.
> 
> Christian, I'd also be glad if you could state your thoughts again (regarding 
> the open issues and the other parts of the UI-review (e.g. wording)). I will 
> attach screenshots for this shortly.
> 
Thanks.
Comment on attachment 293417 [details] [diff] [review]
Hook up unifinder to Lightning & Sunbird - 2nd try

I have a working patch now with a Close-Button and the changed wording ("Search Calendar"). 

I'll post the updated patch once I get the toolbar button PNG from Christian.

Therefore I'm obsoleting this patch.
Attachment #293417 - Attachment is obsolete: true
Attachment #293417 - Flags: ui-review?(christian.jansen)
Attachment #293417 - Flags: review?(philipp)
Because toolbars usually have limited space for icons and text, it would be nice if all icon text in Lightning/Sunbird is only a single word whenever possible.  Thus I would prefer "Search" instead of "Search Calendars".  I think that "Search" is sufficient because the user knows that he's in Calendar Mode and knows that he'll be searching for calendar items.

However, it's not obvious if the user is searching for events or tasks, so if you must use two words, I think that it would be better to use "Search Events" instead of "Search Calendars".
p.s. If you keep the icon text to a single word (e.g. "Search") then you can use the tooltip for a complete description (e.g. "Search for events" or "Search for events in my calendars").  I noticed that Thunderbird and Lightning do this very effectively with the other toolbar buttons.
Please find the icons attached. I've created a set for winstripe and a set for pinstripe.

I suggest to put the "Search" function under "Edit" [1] this location would fit much better to the Mail Mode (Edit -> Find). We may also call this function "Find" instead of "Search", but this should be decided by a native speaker :-)

Regarding the default visibility of the unifinder. IMO it should be switched off.

http://spreadsheets.google.com/pub?key=plCAueWeXt4hSxoRkxWFJCQ
I'm not sure if "Search" or "Find" is an adequate name for the unifinder. Currently we already have menu View -> Search Bar that shows/hides the search bar within the unifinder. At least in my usual Sunbird setup the search bar is hidden and the unifinder displays the Agenda for the next days while the calendar view shows the complete month. 
Attached patch Patch v6 (obsolete) — Splinter Review
This is the newest iteration of my patch. It still has some minor issues, but I'd really like to get some feedback on it from a more experienced developer (Hello Philipp!).

The patch has the following minor issues:
1. The "Find events" menuitem in Thunderbird is located in the View-Menu and 
   not in the Edit-menu. The reason for this is, that the Thunderbird 
   Edit-Menu misses an ID and can therefore not be overlaid. I'm open for 
   suggestions on how to solve this.
2. The unifinder is not disabled at startup. I tried to set the 
   collapsed="true" on the relevant XUL elements, but once those were 
   collapsed I couldn't get them to display anymore. I'm open for suggestions 
   on how to solve this as well.
3. The "Find events"-button has no depressed state. I set the 'checked="true"' 
   attribute, but it didn't work. Any help here is also appreciated.

Philipp, it would be great if I could get some feedback from you on this (or 
from any other knowledgable person). Thanks!
Attachment #293951 - Flags: ui-review?(christian.jansen)
Attachment #293951 - Flags: review?(philipp)
Attached patch Patch v7 (obsolete) — Splinter Review
Same with a minor bug fixed, that I found after further testing. All the issues mentioned in comment 21 for v6 still apply.
Attachment #293418 - Attachment is obsolete: true
Attachment #293419 - Attachment is obsolete: true
Attachment #293951 - Attachment is obsolete: true
Attachment #293978 - Flags: ui-review?(christian.jansen)
Attachment #293978 - Flags: review?(philipp)
Attachment #293951 - Flags: ui-review?(christian.jansen)
Attachment #293951 - Flags: review?(philipp)
Attached patch Patch v8 (obsolete) — Splinter Review
Sorry for another iteration. I just read comment 20. This patch is the same as v7 (comment 22) with the added removal of the "View -> Search Bar"-menuitem in Sunbird.

IMO we don't need that anymore now that we have a menuitem/toolbarbutton for enabling/disabling the unifinder as a whole, allowing us to simplify our menu structure in Sunbird.

I'd like to hear feedback on this, of course.
Attachment #293978 - Attachment is obsolete: true
Attachment #293979 - Flags: ui-review?(christian.jansen)
Attachment #293979 - Flags: review?(philipp)
Attachment #293978 - Flags: ui-review?(christian.jansen)
Attachment #293978 - Flags: review?(philipp)
Comment on attachment 293979 [details] [diff] [review]
Patch v8

Thanks for the patch, Simon. I really like the new behavior. Please change the following 2 minor issues:

- Add a separator on the right hand side of the "Find Event" toolbar item.

- Change the "Find event" toolbar item label to "Find Event"

r=christian

PS: For being compliant with other apps "Find Events" should move to the "Edit" Menu, but this can be changed later.
Attachment #293979 - Flags: ui-review?(christian.jansen) → ui-review+
Newest iteration with Christian's review comments incorporated. I also fixed one minor copy&paste bug that I found in the previous version of the patch.
Attachment #293979 - Attachment is obsolete: true
Attachment #294119 - Flags: ui-review+
Attachment #294119 - Flags: review?(philipp)
Attachment #293979 - Flags: review?(philipp)
Comment on attachment 294119 [details] [diff] [review]
Patch v9 [checked in]

>            <menuitem id="calendar-selectall-menu"
>                      key="key_selectAll"
>                      label="&calendar.selectall.label;"
>                      accesskey="&calendar.selectall.accesskey;"
>                      observes="select_all_command"/>
>+           <menuseparator/>
>+           <menuitem id="menu_showUnifinder"
>+                     type="checkbox"
>+                     label="&showUnifinderCmd.label;"
>+                     command="cmd_show_unifinder"
>+                     accesskey="&showUnifinderCmd.accesskey;"
>+                     checked="true"/>
I'd prefer an id like "calendar-showunifinder-menuitem" or such. Also, please give the menuseparator an id so that extensions can easily insert items after the separator.

>Index: calendar/sunbird/themes/pinstripe/sunbird/calendar.css
If lightning will also have this button at some point, maybe you can already put the css into a file in base? I'm a bit confused why you do have rules in base/themes further down that look quite the same as these, but then again I didn't look too closely at the two rule sets.

>Index: calendar/sunbird/themes/pinstripe/sunbird/jar.mn
> +  skin/classic/calendar/calendar.css
>-   skin/classic/calendar/toolbar-large.png
>-   skin/classic/calendar/toolbar-small.png
>    skin/classic/calendar/prevnextarrow.png
> +  skin/classic/calendar/synch_animated.gif
>    skin/classic/calendar/datetimepickers/left-arrow.png             (datetimepickers/left-arrow.png)
>    skin/classic/calendar/datetimepickers/left-arrow-hover.png       (datetimepickers/left-arrow-hover.png)
>    skin/classic/calendar/datetimepickers/right-arrow.png            (datetimepickers/right-arrow.png)
Was calendar.css not included before somewhere? I just wonder why its needed in this jar file now and not before?
> +       skin/classic/calendar/datetimepickers/minimonth.css         (datetimepickers/minimonth.css)
The same goes for minimonth.css? 



>-<!ENTITY calendar.new.journal.label              "New Journal Item…">
>-<!ENTITY calendar.new.journal.key                "J">
>-<!ENTITY calendar.new.journal.accesskey          "j">
>+<!ENTITY calendar.new.journal.label             "New Journal Item…">
>+<!ENTITY calendar.new.journal.key               "J">
>+<!ENTITY calendar.new.journal.accesskey         "j">
There is no journal functionality, are these strings used somewhere? Go ahead and get rid of them if not.

>+    <command id="cmd_show_unifinder" oncommand="goToggleToolbar('bottom-events-box', 'menu_showUnifinder'); goToggleToolbar('calendar-view-splitter');"/>
>+
Please align the naming schema for these commands, i.e calendar_show_unifinder_command, or since the command seems to toggle the unifinder, maybe calendar_toggle_unifinder_command.


>+
>+.unifinder-closebutton {
>+  list-style-image: url("chrome://global/skin/icons/closetab.png") !important;
>+}
>+
>+.unifinder-closebutton:hover {
>+  list-style-image: url("chrome://global/skin/icons/closetab-hover.png") !important;
>+}
>+
>+.unifinder-closebutton:hover:active {
>+  list-style-image: url("chrome://global/skin/icons/closetab-active.png") !important;
>+}
>+
>+.unifinder-closebutton > .toolbarbutton-text {
>+   display: none;
>+}
Why do these rules need to be !important?


>+
>+.unifinder-closebutton {
>+  list-style-image: url("chrome://global/skin/icons/close.png");
>+  -moz-image-region: rect(0 16px 16px 0);
>+  -moz-appearance: none;
>+  border: none !important;
>+  padding: 2px;
>+}
>+
>+.unifinder-closebutton:hover {
>+  -moz-image-region: rect(0 32px 16px 16px);
>+}
>+
>+.unifinder-closebutton:hover:active {
>+  -moz-image-region: rect(0 48px 16px 32px);
>+}
Fix tabs here, 4 space.



r=philipp with comments explained/fixed.
Attachment #294119 - Flags: review?(philipp) → review+
(In reply to comment #26)
> I'd prefer an id like "calendar-showunifinder-menuitem" or such. Also, 
> please give the menuseparator an id so that extensions can easily 
> insert items after the separator.

Will do.

> >Index: calendar/sunbird/themes/pinstripe/sunbird/calendar.css
>
> If lightning will also have this button at some point, maybe you can 
> already put the css into a file in base? I'm a bit confused why you do 
> have rules in base/themes further down that look quite the same as 
> these, but then again I didn't look too closely at the two rule sets.

Lightning already has a button. The corresponding CSS is in the 
calendar-toolbar.css part of the patch. 

Thinking about your comment, it might make sense to unify the button styles 
in just one file per theme in calendar/base/. I would put this in a followup
bug though, as the patch is already pretty large.

> >Index: calendar/sunbird/themes/pinstripe/sunbird/jar.mn
> > +  skin/classic/calendar/calendar.css
> > +  skin/classic/calendar/synch_animated.gif
> > +  skin/classic/calendar/datetimepickers/minimonth.css         (datetimepickers/minimonth.css)
>
> Was calendar.css not included before somewhere? I just wonder why its 
> needed in this jar file now and not before? The same goes for minimonth.css? 

Those files were included and are still included. The "+" is not part of the 
patch. Please note the preceding space before the "+". I guess it is some 
kind of XUL preprocessor syntax.

> >-<!ENTITY calendar.new.journal.label              "New Journal Item…">
> >-<!ENTITY calendar.new.journal.key                "J">
> >-<!ENTITY calendar.new.journal.accesskey          "j">
> >+<!ENTITY calendar.new.journal.label             "New Journal Item…">
> >+<!ENTITY calendar.new.journal.key               "J">
> >+<!ENTITY calendar.new.journal.accesskey         "j">
>
> There is no journal functionality, are these strings used somewhere? 
> Go ahead and get rid of them if not.

Yes:

http://lxr.mozilla.org/seamonkey/source/calendar/sunbird/base/content/calendar-sets.inc#86
http://lxr.mozilla.org/seamonkey/source/calendar/sunbird/base/content/calendar-menubar.inc#67
http://lxr.mozilla.org/seamonkey/source/calendar/sunbird/base/content/calendar-menubar.inc#68

The menuitem is disabled and has a "<!-- XXX lilmatt: Finish this!" comment 
preceding it. I'll follow your advice and remove the strings and menuitems.
But I'd prefer it, if we could do this in a followup bug.

> >+    <command id="cmd_show_unifinder" oncommand="goToggleToolbar('bottom-events-box', 'menu_showUnifinder'); goToggleToolbar('calendar-view-splitter');"/>
> >+
> Please align the naming schema for these commands, i.e
> calendar_show_unifinder_command, or since the command seems to toggle the
> unifinder, maybe calendar_toggle_unifinder_command.

Will do.

> >+.unifinder-closebutton {
> >+  list-style-image: url("chrome://global/skin/icons/closetab.png") !important;
> >+}
>
> Why do these rules need to be !important?

I copied all those rules from the corresponding rules for the today-pane 
close-button. I did not put much thought into it. I guess I just thought, 
that if Berend thought it was necessary, I'd follow him there.

I'll test if this works without the !important. If it does, I'll remove 
those before checking in.

> >+.unifinder-closebutton {
> >+  list-style-image: url("chrome://global/skin/icons/close.png");
> >+  -moz-image-region: rect(0 16px 16px 0);
> >+  -moz-appearance: none;
> >+  border: none !important;
> >+  padding: 2px;
> >+}
> >+
> >+.unifinder-closebutton:hover {
> >+  -moz-image-region: rect(0 32px 16px 16px);
> >+}
> >+
> >+.unifinder-closebutton:hover:active {
> >+  -moz-image-region: rect(0 48px 16px 32px);
> >+}
>
> Fix tabs here, 4 space.

Will do.

Thanks for the review and I await your feedback on my explanations.
(In reply to comment #27)
> Thinking about your comment, it might make sense to unify the button styles 
> in just one file per theme in calendar/base/. I would put this in a followup
> bug though, as the patch is already pretty large.
Whichever you prefer, we should definitely unify these though.


> Those files were included and are still included. The "+" is not part of the 
> patch. Please note the preceding space before the "+". I guess it is some 
> kind of XUL preprocessor syntax.
Ah yes, sorry. I mistook that for the patch syntax +.

> The menuitem is disabled and has a "<!-- XXX lilmatt: Finish this!" comment 
> preceding it. I'll follow your advice and remove the strings and menuitems.
> But I'd prefer it, if we could do this in a followup bug.
Sure, go ahead. Since we have no Journal features planned currently, I think we should definitely get rid of them. 
I filed bug 409842 and bug 409843 for the followup issues.
Patch and missing parts (removal of unused images, merge conflicts, etc.) checked in on HEAD and MOZILLA_1_8_BRANCH.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Philipp, this patch fixes issue 3 from comment 21. It would be great if you could review it as well. 

I found this solution with the help of Martin Schröder. Credits go to him.
Attachment #294780 - Flags: review?(philipp)
Comment on attachment 294780 [details] [diff] [review]
Patch to enable pressed unifinder-button state when enabled [checked in]

r=philipp
Attachment #294780 - Flags: review?(philipp) → review+
Followup patch checked into HEAD and MOZILLA_1_8_BRANCH.
Attachment #294780 - Attachment description: Patch to enable pressed unifinder-button state when enabled → Patch to enable pressed unifinder-button state when enabled [checked in]
Attachment #294119 - Attachment description: Patch v9 → Patch v9 [checked in]
I just installed the 01/03/08 lightning nightly, and notice that the "Find Event" toolbar button is located in the mail toolbar button set, and cannot be added to the calendar toolbar (unless there is some trick to moving the buttons between toolbars that isn't obvious to me).
Lifesaver2000, thanks for noticing. I screwed up and missed to add one line to our code, that enables the button only on the calendar toolbar. I checked in a followup patch about a minute ago.

The next should contain the patch.
Verified in nightly build 20080108 -> task is fixed.

I found this minor issue: in thunderbird the 'Find Events' entry is a item in the View menu and in sunbird the 'Find Events' entry is a item in the Edit menu, which is correct (see comment #24). I write a new bug for this issue.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.