160019 - First step to a robust bookmarks handling

Status: RESOLVED FIXED

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

Description

[Pierre Chanial]

The patch I will attach proposes a way to simplify (a lot) the bookmark code and
make it robust (move/delete/cut/copy) and maintainable.

WHAT:
1) remove the fork code between trees and the personal toolbar.
2) systematic handling of non permitted actions.
3) services defined as const in one place

WHY:
1) The current bookmark code is quite complicated and rather difficult to
maintain in its current form. All the commands are duplicated in bookmarks.xml,
bookmarksOverlay.js and a bit in personalToolbar.js. None of the version is
bug-free.
2) Wrong actions have been prohibited patch after patch, (copy a folder into
itself, drop in find:bookmarks or file: bookmarks, etc...) some part of the
patch is corrected but the major part isn't.
3) There is no naming convention (RDF, RDFC, etc...). I found a misleading
naming. But the interest here is to make the code much more readable by not
redefining those services everytime we need them.

HOW?
1) The commands were defined in BookmarksUIElement.prototype and
<bookmarks-tree>. They will be removed in bookmarks.xml and ve only defined in a
var called BookmarksCommand. 
A method specific to trees |getTreeSelection| and another one |getPTSelection|
specific to the personal toolbar will return an object containing the selection.
This object will be passed to the methods in BookmarksCommand.
2) the selection will contain information whether the selected item is immutable
or not. The immutability will be computed once in one place. It will also
contain the item parent URI so that the bookmarks will be uniquely identified.
A method |checkTargetFolder| will check if the selection can be dropped (paste,
moved...) in a folder. CM menu, buttons and commands will use this information
to allow or forbid the actions.
3) services, bookmark datasource will be defined in one place at the top of
bookmarksOverlay.js. A better place would obviously be a proper file services.js
but due to redefinition conflicts, I prefer to postpone this issue.

The patch:
It implements only one command in the way I described previously: File bookmarks
renamed as Move Bookmarks (bug 159762) Note: The File Bookmarks command in the
Bookmark menu in the PT or menubar is unrelated to this one and is therefore not
touched. Only the bookmark manager and the CM are affected by the name changing.
Since the patch is just a first step, BookmarksCommand is still assigned to
BookmarksUIElement.prototype. 
In addition, CM does not use yet the facilities provided by the selection to
disable its menuitems.

Comment 1

[Pierre Chanial]

Attachment: Patch v1.0

note: this patch does not break the mozilla bookmark handling, even if it only
deals with |Move Bookmarks|
Comments are welcome!!!

Comment 2

[Pierre Chanial]

Attachment: Patch v1.1

Patch addressing timeless review

Comment 3

[Pierre Chanial]

Attachment: Patch v2.0

Well, here is the last version of my patch. I'll give some precision tomorrow

Comment 4

[Pierre Chanial]

So far, this patch needs still a bit of work, but I would like to gather some
feedback before polishing it.
Basically what it does it:
- gets rid of the duplicated RDF projections (tree and DOM) and do the work with
RDF, only. To do so, I implemented two methods getTreeSelection and
getBTSelection (BT stands for bookmark toolbar) that set up an object
'selection' that does not include tree or DOM representation but instead,
include the selected item and parent RDF node.
That way, I could remove 1200 lines in bookmarks.xml and completely drop the
file personalToolbar.js (500 lines). All the actions are now in BookmarksOverlay.js
- this selection object includes pertinent information, that will prevent silly
actions, that can be easily tweaked.
- actions are performed in only two methods: insertSelection and removeSelection
- implements the editor transaction manager for undoing/redoing bookmark
actions. So far, drag, delete, new folder|bookmark|separator, move actions are
handled. Still need to do the import.

Comment 5

[Pierre Chanial]

also I xblyfied the personal toolbar. Bookmark can now have two light
representations: 
- tree (bookmarks.xml that should be renamed into bookmarksTree.xml)
- toolbar (bookmarksToolbar.xml)
There is still the menu to be done, though
The motivation is clearly for easier use of the bookmark module by clients

The patch removes also the unused files:
- bookmarksTree.js
- bookmarksDD.js

Note: 
- I know that the interdependencies between bookmarksToolbar.xml and
navigatorDD.js still suck but should be adressed when the drop into folder
feature will be widgetized.
- and I am also aware that I did not include the new file bookmarksToolbar.xml
in the proper manner (jar.mn and proper cvs)

Comment 6

[Andrew Hagen]

Dude, you rock.

Comment 7

[Pierre Chanial]

moving to m1.3a, unfortunately, I will have no time to update this patch and
because I can not guarantee to be on the hook to fix the eventual regressions.
This patch will be checked in phoenix, though, quite soon.

Comment 8

[Paolo Marani]

That's the right way to go, dont give up!
I'll closely watch improvement on bookmark handling
hopefully landing in 1.3a

Regards,

Comment 9

[Mike Fedyk]

What is happening with this bug?

Comment 10

[Pierre Chanial]

Sorry, there are still some issues that I have to hammer first. Currently, I
have no time to work on it.
->1.5 (maybe 1.4 hopefully)

Comment 11

[Pierre Chanial]

let's be optimistic -> 1.4a

Comment 12

[Jan Varga [:janv]]

I really look forward to see this checked in.

Comment 13

[Pierre Chanial]

landed in the bookmark branch.
should be ready for 1.4b early

Comment 14

[Jan Varga [:janv]]

The bookmarks branch has landed.
Fixed.

Comment 15

[Andreas Kunz]

This also fixed _at_least_ bug 53422, bug 81895, bug 90567, bug 92317, bug
122199, bug 138523, bug 159762, bug 160019 and bug 165150.
Can a bookmark branch person please resolve them?

Comment 16

[Becki Sanford]

Was this patch supposed to fix a problem pointed out in comment #20 of bug
#90567?

Steps to reproduce:
1) open Bookmarks|Manage Bookmarks
2) select and highlight a bookmark contained in a folder
3) click on "File Bookmarks" button
4) select the SAME folder for the target location and click "OK"

Result: bookmark not displayed in expanded folder view until it is collapsed and
reopened.
Expected Result: the bookmark should be displayed without extra effort.

Comment 17

[Jan Varga [:janv]]

Becki, it works for me with a recent nightly.