Closed Bug 955490 Opened 10 years ago Closed 10 years ago

Add data to 'conversation-loaded' notification to let observers know that the conversation was moved

Categories

(Instantbird Graveyard :: Conversation, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: benediktp, Assigned: benediktp)

References

Details

Attachments

(1 file)

*** Original post on bio 2053 at 2013-07-15 07:36:00 UTC ***

*** Due to BzAPI limitations, the initial description is in comment 1 ***
Depends on: 954607
Attached patch PatchSplinter Review
*** Original post on bio 2053 as attmnt 2597 at 2013-07-15 07:36:00 UTC ***

This is the follow up to bug 954607 (bio 1175) to let observers know that the conversation-loaded event belongs to a conversation that was shown in a different window before. They might want to react differently in this case, e.g. by *not* adding things to the conversation content.
The MathJax-add-on could make use of this as far as I know.

Tested with an own extension (Input history) by reporting when it was attached to a conversation that was carrying this flag in the data parameter of the notification.

No problems expected as the data parameter of the "conversation-loaded" notification doesn't seem to be used from anywhere else yet.
Attachment #8354366 - Flags: review?(clokep)
*** Original post on bio 2053 at 2013-07-15 09:18:01 UTC ***

Thanks for the followup!

Does the data have to be a string? If not, I think I would prefer it if it was simply a boolean (true) in this case, as there is no data set in the other circumstances that conversation-loaded is sent.
Comment on attachment 8354366 [details] [diff] [review]
Patch

*** Original change on bio 2053 attmnt 2597 at 2013-07-15 10:26:26 UTC ***

The data parameter does need to be a string: https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIObserverService#notifyObservers%28%29
Attachment #8354366 - Flags: review?(clokep) → review+
Whiteboard: [checkin-needed]
*** Original post on bio 2053 at 2013-07-15 12:01:55 UTC ***

(In reply to comment #1)

> there is no data set in the other
> circumstances that conversation-loaded is sent.

I was wondering if we should put some data there for the other cases. I suspect it may be useful for add-ons to be able to differentiate if we are displaying a log in the log viewer, previewing a message theme in the pref window, or showing a conversation tab.
*** Original post on bio 2053 at 2013-07-15 12:09:28 UTC ***

(In reply to comment #3)
> (In reply to comment #1)
> 
> > there is no data set in the other
> > circumstances that conversation-loaded is sent.
> 
> I was wondering if we should put some data there for the other cases. I suspect
> it may be useful for add-ons to be able to differentiate if we are displaying a
> log in the log viewer, previewing a message theme in the pref window, or
> showing a conversation tab.

It's already possible to differentiate by looking at the windowtype (eg https://bitbucket.org/aleth/mathjax-addon/src/e7ddef33088541b2df553c7dd18bc606384595fd/bootstrap.js?at=master#cl-110) but of course this would be nicer for add-on authors.

Basically that would mean adding a windowtype check to
http://lxr.instantbird.org/instantbird/source/chat/content/convbrowser.xml#838
Since the convbrowser is not supposed to care about what window it is in, I'm not sure if that should be done?
*** Original post on bio 2053 at 2013-07-15 14:05:47 UTC ***

(In reply to comment #4)

> Basically that would mean adding a windowtype check to
> http://lxr.instantbird.org/instantbird/source/chat/content/convbrowser.xml#838

No, there are other ways.

The way I had in mind would be to add an attribute in convbrowser that would be the 'type of conversation view' and would have 'conversation' as default value.

The log viewer and preference windows would set this attribute to a different value when they initialize their convbrowsers.

The convbrowser would just use the value of this attribute for the notification.


The part that isn't clear to me is: is it worth doing, or given that no add-on seems to have desperately needed it yet, should we just take the current patch and add more stuff later if use cases appear?
*** Original post on bio 2053 at 2013-07-15 22:08:42 UTC ***

We can morph or replace this bug to include the things that flo suggested. It sounds reasonable to pass such information with the notification and since there's no immediate need to land this as soon as possible, we could as well cover these other cases too at once.
Whiteboard: [checkin-needed]
*** Original post on bio 2053 at 2013-10-23 14:15:27 UTC ***

Is this "checkin-needed" or should I go with flo's solution?
*** Original post on bio 2053 at 2013-10-23 16:23:46 UTC ***

(In reply to comment #7)
> Is this "checkin-needed" or should I go with flo's solution?

It would be nice to check it in before 1.5 if you don't have the time for the more complicated idea before then.
Whiteboard: [checkin-needed]
*** Original post on bio 2053 at 2013-11-10 03:20:30 UTC ***

http://hg.instantbird.org/instantbird/rev/e3fb5a5b56d1

Thanks! Can someone add documentation to the wiki about this please?
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.5
*** Original post on bio 2053 at 2013-11-10 09:45:34 UTC ***

Done.
*** Original post on bio 2053 at 2013-11-10 13:58:12 UTC ***

(In reply to comment #10)
> Done.

To be clear, the details on https://wiki.instantbird.org/Instantbird:Notifications:trunk were updated!
You need to log in before you can comment on or make changes to this bug.