359462 - (placescontroller) Redesign PlacesController interaction with places views (was: context menu delete and keyboard delete (select all, cut, copy, etc) doesn't work in places-based history sidebar)

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[(not reading, please use [e-mail] instead)]

context menu delete and keyboard delete doesn't work in places-based history sidebar

this will soon be a regression.

will work on this as soon as the big patch for bug #356487 lands.

Comment 1

[(not reading, please use [e-mail] instead)]

note, other items in the edit menu need to be fixed as well, such as select all.

Fx 2 has some bugs (such as copy not doing the right thing)

I've got a patch started, which leverage's the existing places controller (controller.js), but it needs some work.

I'll attach a rough draft and list the open issues I'm still working on.

Comment 2

[(not reading, please use [e-mail] instead)]

Attachment: rough draft, not complete, includes changes for group by date (bug #358784)

Comment 3

[(not reading, please use [e-mail] instead)]

> and list the open issues I'm still working on.

1) top level delete of date does not work
2) delete of host, when view by host + date, will delete all for that host, not just those in that date!
3) select all
4) copy in fx 2 is enabled, but on paste, it doesn't work (on mac).  fix this in trunk
5) make cut work on trunk, too?
6) fix this dnd issue.  XXX ex [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDragSession.isDataFlavorSupported]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://global/content/nsDragAndDrop.js :: anonymous :: line 424"  data: no]
7) make sure VK_BACK works on the mac

I'll attach a new patch, now that bug #356487 has landed.

Comment 4

[(not reading, please use [e-mail] instead)]

Attachment: same rough draft, not complete.

Comment 5

[(not reading, please use [e-mail] instead)]

Attachment: updated patch, some progress, needs work (both in command updating and more importantly how to delete by date in places C++)

Comment 6

[(not reading, please use [e-mail] instead)]

a brain dump on the current patch:

1)  not happy with the changes i've made to controller.js (the places controller) and how the history side bar uses it.

2) as for the delete by date code, dmills points out that I could generalize and re-use the nsNavHistoryExpire.cpp code, instead of rewriting the code.

Comment 7

[(not reading, please use [e-mail] instead)]

spinning off problems with delete (when grouped by date, or date and site) to bug #363621.

Comment 8

[(not reading, please use [e-mail] instead)]

re-assign to mano.

Comment 9

[Asaf Romano (gone)]

Attachment: v1

The concept of one controller for all views (and then setting the active view in a focus handlers) doesn't work all that well - automated commands updating is done before the focus event is fired. There's a hack in the places-tree binding (and probably in few more places) to work around that which calls updateCommands for the commandDispatcher of window.docuemnt. This means commands were not updated in the parent/child windows (in the sidebar case, that means edit-commands were not updated for the browser-window's edit menu).

So, I got rid of the entire activeView story, meaning views should create and use their owen PlacesController. This works except I didn't yet update views other than the sidebar.

Context-menu code changes here are wrong, my plan is to factor this code out of the places controller.

Comment 10

[Asaf Romano (gone)]

Comment on attachment 248602 [details] [diff] [review]
v1

If no one objects, I would like to land this first and fix other views separately (meaning --enable-places-bookmarks will be broken for few days at the very least). Please ignore the changes to buildContextMenu and LOG.

Comment 11

[Asaf Romano (gone)]

Comment on attachment 248602 [details] [diff] [review]
v1

Don't review this yet.

Comment 12

[Asaf Romano (gone)]

Attachment: more...

Comment 13

[Asaf Romano (gone)]

Attachment: more...

Comment 14

[Asaf Romano (gone)]

Attachment: more...

Comment 15

[Brett Wilson]

I have some thoughts on this, please ping me sometime on how I think transactions should work.

Basically, I think the transactions in the JS "controller" were a bad idea. I think the Nav* services should have functions that return transactions, instead of implementing the function directly, and these transactions should be written in C++ within the service.

Comment 16

[Asaf Romano (gone)]

Transactions code was (almost) not changed here at all.

Comment 17

[Asaf Romano (gone)]

Attachment: more...

Comment 18

[Asaf Romano (gone)]

Attachment: remotely stable

Comment 19

[Asaf Romano (gone)]

Attachment: more...

Fix busted delete folder and copy transactions;

Comment 20

[Asaf Romano (gone)]

Attachment: more...

Comment 21

[Asaf Romano (gone)]

Attachment: fix remaining command-updating issues

Comment 22

[(not reading, please use [e-mail] instead)]

asaf, sorry for the delay.  looking over (and testing) your patch today.

Comment 23

[O. Atsushi (Torisugari)]

Excuse me, about trivial coding style.

|PlacesUtils._uri| and |PlacesUtils._wrapString| need "_" ?

If I understand correctly, by prefix "_" the original authors mean
"the property/method is private" and "to be called only by |this|
or |self|". That's the difference between |activeView| and
|_activeView| etc.

So, _uri(...) -> wrapURI(...) and _wrapString(...) -> wrapString(...),
as they are public utilities now...?

Comment 24

[Asaf Romano (gone)]

Attachment: applies to trunk

Comment 25

[Asaf Romano (gone)]

Attachment: don't initialize views twice

This fixes the on-startup JS error I introduced.

Comment 26

[(not reading, please use [e-mail] instead)]

got a little sidetracked with regression bug #364018, but I'm back on the test / review of this patch now.

Comment 27

[(not reading, please use [e-mail] instead)]

asaf, still working through the patch.  when testing, everything looks good.

one question so far, which did you move this into a timeout?

     else {
       // Set the mode on an existing places window. 
-      organizer.selectPlaceURI(place);
-      organizer.focus();
+      setTimeout(function() {
+        organizer.selectPlaceURI(place);
+        organizer.focus();
+       }, 0);
     }
   },

Comment 28

[Asaf Romano (gone)]

Ignore it, I forgot to remove this when I realized it fixes nothing.

Comment 29

[(not reading, please use [e-mail] instead)]

Comment on attachment 249062 [details] [diff] [review]
don't initialize views twice 

r=sspitzer

sorry for the delay.  the history sidebar on place tests out fine.  I only had a few more questions and requests for spin of bugs.

Comment 30

[(not reading, please use [e-mail] instead)]

1)
 
+    <!-- XXXmano: not yet supported -->
     <command id="placesCmd_reload" 
              label="&cmd.reload.label;" accesskey="&cmd.reload.accesskey;"
              oncommand="PlacesController.reloadSelectedLivemarks();"
              disabled="true"/>
-  </commandset>
-</commandset>
+    <command id="placesCmd_sortby:name"
+             label="&cmd.sortby_name.label;"/>
 
+    <!-- XXXmano: not implemented? -->
+    <command id="placesCmd_rename"
+             label="&cmd.rename.label;"
+             accesskey="&cmd.rename.accesskey;"/>
+#endif
+</commandset>

Can you log a spin off bug on these XXX items?
 
2)
 
+#include commands.inc
   <!-- XXX fix me:  
     context menu delete and cmd_delete are not working yet.  see bug #359462
     -->

I think you can remove my XXX fix me comment, as you have fixed this, right?       

3)

+    /**
+     * XXXmano: this used to be a hack which allowed using few places commands
+     * when the search field is focused. Re-implementing it would probably
+     * require adding a controller to the searchbox which would forward over
+     * those commands to the places tree controller.
+     *
+     * // Initialize the active view so that all commands work properly without
+     * // the user needing to explicitly click in a view (since the search box is
+     * // focused by default). 
+     * PlacesController.activeView = this._places;
+     */

do we need a spin of bug on restoring this hack?

4)

+    <implementation implements="nsIDOMEventListener">
       <constructor><![CDATA[
         // Support an asyncinit attribute that causes the view to populate 
         // itself only after the window has been shown. This is to ensure we
         // do not regress browser window show time (Ts/Txul)
+#if 0
         if (this.hasAttribute("asyncinit")) {
           var self = this;
           //setTimeout(function() { self._init(); }, 0);
         }
         else
+#endif
           this._init();
       ]]></constructor>
       
Can you explain why you turned this code off?

Comment 31

[Asaf Romano (gone)]

(In reply to comment #30)
> 1)

Sure.

> 2)
> 
> +#include commands.inc
>    <!-- XXX fix me:  
>      context menu delete and cmd_delete are not working yet.  see bug #359462
>      -->
> 
> I think you can remove my XXX fix me comment, as you have fixed this, right?    
> 

There's still some work to do here (delete of a anything but folders is not undo-able at the very least). Spin of, I guess.

> 3)

> do we need a spin of bug on restoring this hack?

Maybe, we should investigate this when we implement Fx2-bookmarks-manager functionality.

> 4)

> Can you explain why you turned this code off?
> 

It's not me, it's Ben (see setTimeout prefixed with //). These hacks were used to improve/fake Ts, we should spin of this as well (and see how much it actually affects Ts either way).

Comment 32

[Asaf Romano (gone)]

Filed: bug 364643, bug 364644.

Comment 33

[Asaf Romano (gone)]

Attachment: as checked in

mozilla/browser/base/content/browser-places.js 1.14
mozilla/browser/base/content/browser-sets.inc 1.86
mozilla/browser/base/content/browser.js 1.747
mozilla/browser/base/content/browser.xul 1.331
mozilla/browser/base/content/global-scripts.inc 1.12
mozilla/browser/components/places/jar.mn 1.29
mozilla/browser/components/places/content/bookmarkProperties.js 1.26
mozilla/browser/components/places/content/bookmarkProperties.xul 1.15
mozilla/browser/components/places/content/commands.inc 1.21
mozilla/browser/components/places/content/context.inc 1.12
mozilla/browser/components/places/content/controller.js 1.102
mozilla/browser/components/places/content/history-panel.xul 1.2
mozilla/browser/components/places/content/menu.xml 1.59
mozilla/browser/components/places/content/places.js 1.68
mozilla/browser/components/places/content/places.xul 1.50
mozilla/browser/components/places/content/toolbar.xml 1.65
mozilla/browser/components/places/content/tree.xml 1.44
mozilla/browser/components/places/content/treeHelpers.js 1.4
mozilla/browser/components/places/content/utils.js initial revision: 1.1

Comment 34

[Asaf Romano (gone)]

@O. Atsushi: I missed comment 23, mind to file a bug?

Comment 35

[Jim Jeffery not reading bug-mail 1/2/11]

This appears to have broken a couple of extensions:
Console2
Gmail Notify
Adblock Plus

See forum thread:
http://forums.mozillazine.org/viewtopic.php?p=2662949#2662949
Posts by Littlemutt, and -fullmetaljacket-

Comment 36

[Asaf Romano (gone)]

Double check checkins ranges, I doubt any of those extensions rely on the places controller.

(And, please, file a bug).

Comment 37

[Jim Jeffery not reading bug-mail 1/2/11]

(In reply to comment #36)
> Double check checkins ranges, I doubt any of those extensions rely on the
> places controller.
> 
> (And, please, file a bug).
> 
Yikes!, your right, missed a check-in, it looks more likely as the culprit.

Sorry for the spam

Comment 38

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv