Closed Bug 372829 Opened 17 years ago Closed 17 years ago

Integrate Unifinder into Lightning as it is in Sunbird

Categories

(Calendar :: Lightning Only, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: giermann, Assigned: michael.buettner)

References

Details

Attachments

(2 files, 3 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1.2) Gecko/20070219 Firefox/2.0.0.2
Build Identifier: Lightning 0.3.1 (build 2007021403)

There is no way to display an event list in Lightning, nor is there a chance to search for events.

It would be quite easy to integrate the Sunbird Unifinder into Lightning with all the functionality!


Reproducible: Always




I created a DIFF file on the release-source to do the integration.

Unfortunately I could not test the build, but I integrated it into the JARs manually, which worked fine.

There should also be a generic discussion, if the add-on functions and global variables for the Unifinder scripts should be placed somewhere else and if the "real" CalenderWindow.js is used instead of my "imitation".

You should note, that no modification to the Unifinder script itself was made!
Attachment #257516 - Flags: ui-review?(mvl)
Attachment #257516 - Flags: second-review?(jminta)
Attachment #257516 - Flags: first-review?(shaver)
Please note, that I created this enhancement together with the integration of UnifinderToDo instead of the task-list, as described in bug 372830.

Maybe some modifications are needed to integrate both, as I manually separated them to submit this bug.
Version: unspecified → Lightning 0.3.1
Attachment #257516 - Flags: first-review?(shaver) → first-review?(lilmatt)
Please be aware that I have some tough deadlines for school coming up (in addition to the fact that my laptop is being repaired) so I probably won't be able to give this code a good review for at least 2 weeks.  You may want to consider a different reviewer.
Attachment #257516 - Flags: second-review?(jminta) → second-review?(dmose)
Comment on attachment 257516 [details] [diff] [review]
Enhancements to include Unifinder to Lightning

dmose, is no longer actively working on Calendar code. Moving review to mvl.
Attachment #257516 - Flags: second-review?(dmose) → second-review?(mvl)
Comment on attachment 257516 [details] [diff] [review]
Enhancements to include Unifinder to Lightning

This does not need a 2nd review anymore, since lilmatt is a module peer.
Attachment #257516 - Flags: second-review?(mvl)
Whiteboard: [cal-ui-review needed]
Summary: Please integrate Unifinder into Lightning as it is in Sunbird → Integrate Unifinder into Lightning as it is in Sunbird
will this be added to lightning, possibly as an option? 
Whiteboard: [cal-ui-review needed] → [cal-ui-review needed] [qa discussion needed]
Status: UNCONFIRMED → NEW
Ever confirmed: true
Christian, I assume this conflicts with what you're planning regarding the general view enhancements and all related stuff. Any comments on this?
Attachment #257516 - Flags: first-review?(lilmatt) → review?(lilmatt)
Whiteboard: [cal-ui-review needed] [qa discussion needed] → [cal-ui-review needed]
Comment on attachment 257516 [details] [diff] [review]
Enhancements to include Unifinder to Lightning

removing code review request until we get this through ui-review
Attachment #257516 - Flags: review?(lilmatt)
Comment on attachment 257650 [details]
Screenshot of Unifinder trees in Lightning

Christian, since Mickey specifically asked you in comment 8 what you think about this approach I think it makes sense to let you do the UI-review.
Attachment #257650 - Flags: ui-review?(christian.jansen)
Attachment #257516 - Flags: ui-review?(mvl)
For anyone who is still interested in this function, have a look to my extension 'ltnPlus':
https://addons.mozilla.org/thunderbird/addon/5285


So it is not necessary to get the ui-review, but it demonstrates that this is possible by a simple overlay (with a few modifications: Bug 386714).
OS: Windows XP → All
Hardware: PC → All
Whiteboard: [cal-ui-review needed]
Assignee: nobody → giermann
Status: NEW → ASSIGNED
Attached patch patch v1 (obsolete) — Splinter Review
This is an updated version of the patch with all the bits and pieces resolved to make the integration as clean as possible. Specifically, I played close attention to combine the new commands with the already existing ones (new,delete,modify) and to not duplicate any code (gCalendarWindow and such).

I'm not sure how this integrates with the agenda (which will move from where it is currently located to the today pane). Probably parts of this patch can be taken for the new agenda or we take some of this stuff for the search feature that Christian mentioned on the F2F meeting. In any case, I think this patch is a valuable piece of the puzzle.

Credits go to Sven for the initial version, thanks for the work. Hope you don't mind me stealing the bug ;-)
Assignee: giermann → michael.buettner
Attachment #257516 - Attachment is obsolete: true
Attachment #272472 - Flags: ui-review?(christian.jansen)
Attachment #272472 - Flags: review?(philipp)
Attachment #257650 - Flags: ui-review?(christian.jansen)
(In reply to comment #13)
> Credits go to Sven for the initial version, thanks for the work. Hope you don't
> mind me stealing the bug ;-)

Never mind! I'm glad, that someone "professional" takes this bug...
My solution was just "get it working and forget it". Indeed I found, that with version 0.5 some overheads are not needed any more (gCalendarWindow).

Furthermore this one was some kind of proof-of-concept for me, to show that integration of the existing (!) Unifinder is quite easy. Indeed it's cleaner to edit this line in Unifinder.js and maybe to pay attention to Lightning in future versions of Unifinder ;-)
Comment on attachment 272472 [details] [diff] [review]
patch v1

First of all, I'm sorry for the delay. I'm not at home right now and have limited internet access. I'll assume you've tested this. 

+#expand    skin/classic/calendar/calendar-main.css      (themes/__THEME__/calendar-main.css)
+++ mozilla/calendar/base/themes/pinstripe/calendar-main.css	2007-07-16 14:13:32.812500000 +0200
+++ mozilla/calendar/base/themes/winstripe/calendar-main.css	2007-07-16 14:13:32.812500000 +0200
These files are the same, can we put them into base/themes/common until this changes? Also, please clean up the style in these files. (i.e brackets on same line as selector, no spaces after property names


+function ltnSelectAllEvents()
No need to have two functions that do exactly the same. Put resources/content/calendar.js' selectAllEvents() function somewhere sensible into base and use that directly from lightning.

 
+function initializeUnifinderList()
+{
+  // get the unifinder going. In order to make the unifinder a self-contained
+  // component it could also listen to the 'load' event itself instead of relying
+  // sombody else calling it at the right time.
+  prepareCalendarUnifinder();
+
+  document.getElementById("messengerWindow").addEventListener("unload", finishCalendarUnifinder, false);
+}
Here you note that you *could* make it self contained, but at the end, you do set up the load listener. To keep the load/unload listeners together, go ahead and setup the unload listener down  with the load listener, or is there a reason you are using messengerWindow instead of window?

Also, I if you want to make it really self-contained, it might be smart to put everything that is unifinder specific into a separate file to be put in base/content/calendar-unifinder.js/xul or such and loading it via an overlay. This also means you only need to add the popup menu once (in the overlay). I'm fine with a followup bug though.


+// TODO: find out why we just can't hook up ltnOnLoad(). It seems that
+//       getCompositeCalendar() doesn't like to be called that early.
I believe this has to do with the calendar manager. In lightning, the calendar manager is loaded differently. My patch to unify the calendar list should take care of that.

   <commandset id="calendar_commands">
+    <command id="new_command" oncommand="createEventWithDialog(ltnSelectedCalendar());"/>
+    <command id="new_todo_command" oncommand="createTodoWithDialog(ltnSelectedCalendar());"/>
     <command id="modify_command" oncommand="modifyEventWithDialog(currentView().getSelectedItems({})[0])" disabled="true" disabledwhennoeventsselected="true"/>
+    <command id="delete_command" oncommand="ltnDeleteSelectedItem( )" disabled="true" disabledwhennoeventsselected="true"/>
+    <command id="select_all_command" oncommand="ltnSelectAllEvents()"/>
+    <command id="go_today_command" oncommand="goToToday()"/>
I've had the same idea in my unify calendar list patch. I decided to prefix all (changed) commands with calendar_, to make sure new_command isn't at some point used by thunderbird. I can take care of that in my patch though if you like.

+  content/calendar/unifinder.js    (/calendar/resources/content/unifinder.js)
I would prefer moving this file into base while you are at it, then name it something like calendar-unifinder.js.


Most of this stuff is refactoring. I think it shoud be done, but I'm fine with an extra bug for that if you like. r- for now,  awaiting your decision and to fix the other issues.
Attachment #272472 - Flags: review?(philipp) → review-
Version: Lightning 0.3.1 → unspecified
Attached patch patch v2 (obsolete) — Splinter Review
This version of the patch makes the unifinder a self-contained overlay. New files that are getting introduced:

/m/c/base/content/calendar-unifinder.js
/m/c/base/content/calendar-unifinder.xul
/m/c/base/themes/*instripe/calendar-unifinder.css

This makes /m/c/resources/content/unifinder.js obsolete and removes the xul content from /m/c/sunbird/base/content/calendar.xul. The rest of the patch is just jar.mn & chrome.manifest magic.

From a high-level perspective, bringing the unifinder to lightning fits exactly to 'Local Search: Date range, tags/categories, text' which is item 33 on the roadmap, siehe [1].

Christian, I know that you want to somehow link Thunderbird's search-box to the unifinder, which is something this patch currently does *not* address. Nevertheless, I suggest that we land this patch as it currently stands as I assume it gets instantly bit-rotten (it touches the whole unifinder file). We can easily link the search-box in a spin-off bug if you don't mind.

[1] http://spreadsheets.google.com/pub?key=p6sFBIaPPT7tWR73DtdIkBA
Attachment #272472 - Attachment is obsolete: true
Attachment #281630 - Flags: ui-review?(christian.jansen)
Attachment #281630 - Flags: review?(philipp)
Attachment #272472 - Flags: ui-review?(christian.jansen)
Blocks: 379100
No longer depends on: 379100
Target Milestone: --- → 0.9
Comment on attachment 281630 [details] [diff] [review]
patch v2

ui=christian.
Open Issues:

 - TB Search should be wired up correctly
 - We need a button & menu entry for switching the unifinder off in the calendar mode
 - ...
Attachment #281630 - Flags: ui-review?(christian.jansen) → ui-review+
(In reply to comment #17)
> - TB Search should be wired up correctly
> - We need a button & menu entry for switching the unifinder off
I'm going to address each of these issues in separate spin-offs bugs in order to first get the grunt work done first.
Just some UI cents from me, and don't know if this has already been mentioned:
Could we hide the unifinder results if the search line is empty? I don't like that empty box; it just takes space.
(In reply to comment #19)
> Just some UI cents from me, and don't know if this has already been mentioned:
> Could we hide the unifinder results if the search line is empty? I don't like
> that empty box; it just takes space.
Probably you should just try out the patch by yourself (or refer to Sunbird). You can freely drag the splitter vertically in order to find a height that fits your needs. It can also be collapsed (just as any other pane, e.g. folder pane, today pane, etc.). Furthermore, the unifinder lists the items that are within a given range and (optionally) contain the search string. That's why I'm not sure how we can easily integrate this with the search box as it additionally displays items in a given time range. But to answer the question, just collapse the unifinder and you're done.
Mickey, I've applied that patch of course. Sure, I could manually collapse it, but that's not my point: I just don't like space to be wasted unless I am really searching for something.
- Duplicated information makes IMO little sense (if the search line is empty).
- Empty results box (if nothing is found) takes IMO too much space; a one-liner stating "No results." would be sufficient.
I'd prefer some smart auto-collapsing resp. auto reduction in size for those cases.

Nevertheless, these are just my comments. I trust in Christian to find a good solution.
(In reply to comment #21)
> - Duplicated information makes IMO little sense (if the search line is empty).
Well, I wouldn't call this 'duplicated information'. You have several options to display some or all events in the Unifinder, independent from the current view.
I started to adopt this from Sunbird, because I was asked by users for a list of events... In Lightning it was not possible to get an overview of all events or to search for a specific event.
With the Unifinder you get the 3 pane view, as it exists in Thunderbird. You can compare this to the message list of TB, as an overview to the detailed day/week/month view in 'calendar-view-box'.

> - Empty results box (if nothing is found) takes IMO too much space; a one-liner
> stating "No results." would be sufficient.
As stated above, this is the default behaviour. You also do not have these functionalities in TB's search box...

I agree with Mickey, that one could set the height, as desired - or to remove it completely. I also agree that it would be easier to have a menu item in 'View' menu and maybe a toolbar button, but that will be covered in a spin-off.
Attached patch patch v3Splinter Review
I took care of my review comments myself by presenting a new patch version. It is basically the same patch, but:

* Fitted calendar-unifinder.(xul|js) to calendar style
* Changed the splitter between unifinder and view to 5px, to fit TB style
* Some optimization in calendar-unifinder.js

Functionally, the old patch was fine and works as expected, so no changes there. Changes are mostly style and fixing old code.

Some other things I think should be changed but didn't since its your bug:

* the CSS files don't differ between winstripe and pinstripe. I think they should go into the common directory. The same goes for the unifinder-todo, but thats definitly out of scope.

* I noticed some other style mismatches between lightning and thunderbird that should  be taken care of in a spin-off bug.
Attachment #281630 - Attachment is obsolete: true
Attachment #281945 - Flags: review?(michael.buettner)
Attachment #281630 - Flags: review?(philipp)
(In reply to comment #23)
> I took care of my review comments myself by presenting a new patch version.
Thanks, this is indeed a huge time saver...

> the CSS files don't differ between winstripe and pinstripe. I think they
> should go into the common directory. The same goes for the unifinder-todo, but
> thats definitly out of scope.
I just decided to keep it that way since sunbird used separate files for the styles. This leave the option to have different styles for mac, but if we find it more convenient we can of course move it to the common directory. I would like to keep it the way it is for now.
Comment on attachment 281945 [details] [diff] [review]
patch v3

r=mickey per previous comment.
Attachment #281945 - Flags: review?(michael.buettner) → review+
Keywords: checkin-needed
Keywords: checkin-needed
Whiteboard: [checkin-needed after 0.7]
Target Milestone: 0.9 → 0.8
patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed after 0.7]
Verified with Lightning 2007102923
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.