SeaMonkey sidebar code needs cleanup, to make it possible to add FF sidebar API

RESOLVED WONTFIX

Status

SeaMonkey
Sidebar
RESOLVED WONTFIX
11 years ago
8 years ago

People

(Reporter: Manuel Reimer, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

46.78 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; de-AT; rv:1.8.1.6) Gecko/20070809 SeaMonkey/1.1.4
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; de-AT; rv:1.8.1.6) Gecko/20070809 SeaMonkey/1.1.4

There are several smaller issues with the sidebar, but the most important one is, that the whole code is complex and difficult to understand.

I already rewrote several parts.

The new code uses two arrays to hold the list data and a value "scrollpos" to identify the position, the user scrolled to. The old code used an "in-view" attribute, which had to be set/unset based on the scroll position.

This way, it was also possible to fix several smaller issues, like not restoring the last selected panel, if restoring the last selected panel requires to scroll the list.

The new code also includes the whole Firefox-API for the sidebar, which makes it easier to port extensions. One thing, I wasn't sure about, was the name of the broadcasterset. Firefox uses a different name there, so the extension developer has to create an extra overlay rule for SeaMonkey. Maybe we should add an empty broadcasterset with the ID, also used by Firefox, to our browser?

Reproducible: Always
(Reporter)

Comment 1

11 years ago
Created attachment 281111 [details] [diff] [review]
My first patch

This is the first version of my patch. This one should be tested and checked in first, so the people, who are interested in fixing sidebar bugs, may fix them on the new code.

Comment 2

11 years ago
You should ask someone for review on this, probably Karsten (Mnyromyr) as a reviewer and Neil Rashbrook for super-review make sense there.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Updated

11 years ago
Attachment #281111 - Flags: superreview?(neil)
Attachment #281111 - Flags: review?(mnyromyr)

Comment 3

11 years ago
Comment on attachment 281111 [details] [diff] [review]
My first patch

Manuel, do you see any chance of splitting this up into smaller pieces?

Comment 4

11 years ago
Comment on attachment 281111 [details] [diff] [review]
My first patch

>-const NC = "http://home.netscape.com/NC-rdf#";
>+const sidebarNC = "http://home.netscape.com/NC-rdf#";
sidebarNC isn't correct... if you don't like NC then NC_NS or sidebarObj.nc

>+  if (first_in_view == 0)
>+    upbutton.disabled = true;
>+  else
>+    upbutton.disabled = false;
Use upbutton.disabled = first_in_view == 0;

>+      // The hide and show of 'sidebar-panels' should not be needed,
>+      // but some old profiles may have this persisted as hidden (50973).
>+      this.node.removeAttribute('hidden');
Fortunately no longer a problem on trunk :-)

>+    goPrefBar.dump("in_view" + in_view);
???

>+  // sidebarObj.never_built == gSidebarMustInit???
I think you ought to figure that one out yourself ;-)

>-    var datasource = RDF.GetDataSource(sidebarObj.datasource_uri);
>-    datasource.QueryInterface(Components.interfaces.nsIRDFRemoteDataSource).Refresh(true);
>+    sidebarObj.datasource = RDF.GetDataSourceBlocking(sidebarObj.datasource_uri);
I think that at any time where datasource_uri is defined then datasource should also be defined.

-function SidebarNavigate(aDirection)
-{
+function SidebarNavigate(aDirection) {
As Mnyromyr mentioned, there are too many changes going on for us to be able to figure them all out... these whitespace changes don't help.
Attachment #281111 - Flags: superreview?(neil) → superreview-
(Reporter)

Comment 5

11 years ago
Created attachment 284293 [details] [diff] [review]
Second patch

I fixed the things found by Neil and removed the Firefox API from this file. The API will now be moved to a second bug, to simplify this patch.

I don't request superreview, as this definetly still needs work, but it would help if someone would at least review this state of my work.
Attachment #281111 - Attachment is obsolete: true
Attachment #284293 - Flags: review?(neil)
Attachment #281111 - Flags: review?(mnyromyr)
(Reporter)

Updated

11 years ago
Summary: Firefox sidebar API should be added to the SeaMonkey sidebar, code needs cleanup. → SeaMonkey sidebar code needs cleanup, to make it possible to add FF sidebar API
(Reporter)

Updated

11 years ago
Blocks: 399310
Manuel, Neil,
Are you still working on this ?
Assignee: sidebar → nobody
QA Contact: sidebar
Version: unspecified → Trunk
(Reporter)

Comment 7

10 years ago
I don't think that it's critical, but definetly I think I'll continue the work on sidebar, as I really like the GUI, but think that the backend needs cleanup.

One major problem will be to get a patch someone is willing to review. It can't be done with a simple patch. Maybe it will even be the best idea to rewrite the whole backend code or at least big parts of it.
(Reporter)

Updated

8 years ago
No longer blocks: 399310
(Reporter)

Comment 8

8 years ago
Firefox sidebar API already has been added now using a "workaround".
SeaMonkey sidebar code doesn't need "cleanup", it needs to be replaced at all. This bug is about replacing the sidebar code: Bug 613971

Marking this one WONTFIX
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → WONTFIX
(Reporter)

Comment 9

8 years ago
For those who are interested: This is the bug with the "Firefox sidebar API workaround": Bug 399310
(Reporter)

Updated

8 years ago
Attachment #284293 - Flags: review?(neil)
You need to log in before you can comment on or make changes to this bug.