Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[customize-titlebar] Update treeView code not to use nsISupportsArray #121

Closed
xabolcs opened this issue Mar 9, 2013 · 15 comments
Closed
Assignees
Milestone

Comments

@xabolcs
Copy link
Collaborator

xabolcs commented Mar 9, 2013

nsISupportsArray is going to be removed soon.
If it lands it will not break the Customize Titlebar's treeView - introduced in NTT 3.4!

The nsITreeView fix landed almost a week ago.

Please note about comment 61:

neil wrote at 2013-02-13 00:30:33 PST:
(In reply to alexander surkov from comment 60)

(From update of attachment 713196)

+  getRowProperties: function(aIndex, aProperties) { return ""; },
+  getCellProperties: function(aIndex, aCol, aProperties) { return ""; },
+  getColumnProperties: function(aCol, aProperties) { return ""; },

it's for backward compatibility, right?
No, forward compatibility; the current code doesn't need you to do anything, the new code expects you to return a string.

You could also check out DOM Inspector changeset from comment 89!

As you can see, the fix should be easy.

@whimboo
Copy link
Contributor

whimboo commented Mar 11, 2013

Thanks for the heads-up, even I don't think it will be done in the short term. But yes, we should have this piece fixed and be prepared for a quick release. Lets try to also get as many of the other fixes landed by then.

@tonymec
Copy link
Contributor

tonymec commented Mar 12, 2013

nsISupportsArray has already been removed from nsITreeView on trunk, which breaks ChatZilla. Whoever is waiting to kill it everywhere may be waiting on its removal from various Firefox modules, they won't wait on an extension, even a popular one like NTT. I think it will be done in the very short term, my guess is Fx22, or Fx23 at the latest.

@whimboo
Copy link
Contributor

whimboo commented Mar 12, 2013

So lets get this done asap so we can land a quick follow-up release together with the compatibility pull.

@xabolcs
Copy link
Collaborator Author

xabolcs commented Mar 12, 2013

@tonymec could you test pull #104 again checking for any nsISupportsArray related regression?

I think I did it for recent Firefoxes, but I didn't find anything special - customizing titlebar was working.
I will check the ChatZilla bug to see how it breaks.

@xabolcs
Copy link
Collaborator Author

xabolcs commented Mar 12, 2013

Reading through ChatZilla bug and testing chatzilla with Nightly and Aurora, I think this is not a hard blocker, but a nice-to-have issue. :)

@whimboo
Copy link
Contributor

whimboo commented Mar 13, 2013

So what's actually broken in our case?

@tonymec
Copy link
Contributor

tonymec commented Mar 13, 2013

@xabolcs I have now built an xpi (3.4pre) from commit xabolcs/nightlytt/d124915 . Should I install that or is the current 3.5pre from mozilla/nightlytt good enough? And what do you want me to test in particular? (what steps should I take? what should I look out for?)

@xabolcs
Copy link
Collaborator Author

xabolcs commented Mar 13, 2013

Hi All!
Sorry for the silence.

@tonymec yes, that 3.4pre xpi should you test!
That xpi is a preview of titlebar support of SeaMonkey, and the customizing window uses TreeView.
So you should exercise it: scrolling the view, clicking on the elements, resizing the columns, etc.

As I previously wrote, interestingly I didn't find any regression here.
Looking at the blockers of the TreeView bug they are all styling bug.
And currently NTT doesn't apply any style to that TreeView.

@whimboo
I think there are no feature break (like the pastebin or the screenshot upload issues) here,
but it would be nice to let be the code conform with the new nsITreeView requirements.

@whimboo
Copy link
Contributor

whimboo commented Mar 13, 2013

Thanks a lot! So we really don't have to release a hotfix release and can go with our usual schedule.

@xabolcs
Copy link
Collaborator Author

xabolcs commented Mar 13, 2013

It seems like we doesn't have to release a hotfix, but please wait tonymec's feedback.

@xabolcs
Copy link
Collaborator Author

xabolcs commented Mar 13, 2013

Btw. the trivial patch is done, but not yet and just pushed to my repo.

@tonymec
Copy link
Contributor

tonymec commented Mar 14, 2013

@xabolcs : Short answer: AFAICT, it works the way it should.

Long answer: Adding items manually in the input box at top adds them to the title once I click [OK]. An [Apply] button would be a nice touch but is not essential. Clicking a line in the list adds that item (preceded by a space) at the end of the title in the input box (not where I last put the | insert marker in that box; becomes effective after [OK]). Scrolling works normally, after I resized the dialog height to less than the total height of its contents. Resizing columns works normally. Clicking the column headings does nothing (I tried to see if it would sort the lines according to that column. Not sorting is OK. Sorting badly would be wrong).

Now back to nightlytt-3.5pre-201303092346.xpi from my local clone of the mozilla/nightlytt repo (timestamp in CET, zone +0100 "Central Europe").

@xabolcs
Copy link
Collaborator Author

xabolcs commented Mar 14, 2013

Thank you @tonymec for testing! :)

Labeling as enhancement.

@tonymec
Copy link
Contributor

tonymec commented Mar 14, 2013

P.S. Tested with
Mozilla/5.0 (X11; Linux x86_64; rv:22.0) Gecko/20100101 Firefox/22.0 SeaMonkey/2.19a1 ID:20130313082245 c-c:59251f378743 m-c:b98d7eb83da6
an hourly build on the "bad" side of the cZ styling bug. Note also that in SeaMonkey, ${Changeset} is the comm-central changeset (or comm-aurora, comm-beta etc.); I got the m-c changeset by copy-paste from about:buildconfig.

@xabolcs
Copy link
Collaborator Author

xabolcs commented Mar 19, 2013

Fixed by commit ba25da4 above.

@xabolcs xabolcs closed this as completed Mar 19, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants