Expose tab 'rename' and 'move' events.

RESOLVED INCOMPLETE

Status

Add-on SDK
General
P2
enhancement
RESOLVED INCOMPLETE
4 years ago
3 months ago

People

(Reporter: Ryan Stein, Assigned: irakli)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
This is a feature request for 'rename' and 'move' events for the SDK Tabs module. I have documented what changes are needed for this here: https://github.com/RyanStein/addon-sdk/commit/85cd301479e79cfb2359b108301c3f14128b8e5b

This is my first time contributing to Mozilla, so please let me know if you have any questions or feedback.
Awesome! The next step is to make a pull request and link it here.

Word of warning: due to holiday closure most Mozillians will be off work starting tomorrow, until Jan 2.
(Reporter)

Comment 2

4 years ago
The pull request has been sent: https://github.com/mozilla/addon-sdk/pull/1331

Happy holidays!
Created attachment 8351047 [details] [review]
GH PR 1331

Pinging Irakli for review for API additions
Attachment #8351047 - Flags: review?(rFobic)
Comment on attachment 8351047 [details] [review]
GH PR 1331

Er, just feedback for API additions, I can do the review
Attachment #8351047 - Flags: review?(rFobic) → feedback?(rFobic)
Comment on attachment 8351047 [details] [review]
GH PR 1331

Looks good to me, only thing I'm not sure about is "rename" seems like an
awkward event name, specially since we have `tab.title` rather than `tab.name`.
I would vote for `titleChange` event instead.

I'm fine with keeping event name `rename` if native English speaker like yourself (@jsantell) thinks `rename` is more appropriate.
Attachment #8351047 - Flags: feedback?(rFobic) → feedback+

Updated

4 years ago
Priority: -- → P2
Sorry this has taken so long to get to!

Looks good for the most part, but some changes:

* `rename` event should be `titlechange` (with the constructor handler being `onTitlechange`). `rename` sounds like an active event that was user-initiated, and title change is in line with the title property.

* We can get rid of the documentation in the repository -- they're now independently on MDN (https://developer.mozilla.org/en-US/Add-ons/SDK/High-Level_APIs/tabs) and once this lands in master, can add the appropriate new events there with notes that they are FF31+

* Would like to see tests for when the `titlechange` event is NOT fired -- there's some logic in there that I'd like to see tested to see when changing data on a tab or scenarios where this event shouldn't be fired actually confirms that the event isn't fired. the `move` event probably doesn't need this as there's less logic on our end for that.

Again, sorry this took so long to get to -- ping me with feedback/review/needinformation on here though to make sure I have it in my queue!
Flags: needinfo?(ryan.stein)
Flags: needinfo?(ryan.stein)
Can we leave this to a third party module?
Blocks: 821779
Flags: needinfo?(rFobic)
(In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #7)
> Can we leave this to a third party module?

I think it's absolutely reasonable to have it in core, I'll take over this patch as contributor clearly lost interest in finishing it.
Assignee: nobody → rFobic
Flags: needinfo?(rFobic)
https://bugzilla.mozilla.org/show_bug.cgi?id=1399562
Status: UNCONFIRMED → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.