Remove nsIEditorObserver

RESOLVED WONTFIX

Status

()

Core
Editor
RESOLVED WONTFIX
5 years ago
5 years ago

People

(Reporter: Ehsan, Assigned: capella)

Tracking

({addon-compat, dev-doc-complete})

Trunk
mozilla18
x86
Mac OS X
addon-compat, dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=ehsan][lang=c++])

Attachments

(1 attachment, 1 obsolete attachment)

We don't use the nsIEditorObserver interface for anything, it's time for it to go away.
(Assignee)

Updated

5 years ago
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Ehsan, are you sure it's unused? nsEditor::NotifyEditorObservers seems to call the observers...
(Assignee)

Comment 2

5 years ago
Right ... there's some code in the function that might have to stay after yanking out the notify loop proper ... ?

But then There's the EditAction() function inherited in nsTextEditorState.cpp ... just pulling it, the nsIEditorObserver.idl and Add/Remove Notifications from nsIEditor.idl won't work ... tried and failed various mochitests ...

https://tbpl.mozilla.org/?tree=Try&rev=3a3593c8e594

Which I also re-create locally ... haven't gotten further / debugged - and will be away for close to a week ...
I'm sorry, I was extremely vague when I filed this bug.

The reason that I want nsIEditorObserver to go away is that it is a useless abstraction which is too generic without any good reason.  The only thing which uses it is nsTextEditorState.  I think we should remove nsIEditorObserver, add an abstract base class called EditActionListener, and allow the editor to hold a pointer to it and just call its EditAction when we currently would notify the observers.

That way, nsTextEditorState could inherit from that abstract base class and implement EditAction the same way that it does today, and then it can register and unregister itself on the nsEditor object, and nsEditor would only need to hold a single pointer to the listener since there will only ever be 0 or 1 edit action listeners.
(Assignee)

Comment 4

5 years ago
Created attachment 661047 [details] [diff] [review]
Patch (v1)

Asking for feedback as you have time. The code works / and tests out, but could be "prettier" I'm sure.
Attachment #661047 - Flags: feedback?(ehsan)
Comment on attachment 661047 [details] [diff] [review]
Patch (v1)

Review of attachment 661047 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great!!!

I have a few comments on the patch to make it better, but you basically did the right thing throughout the patch.  Good job!  :-)

::: content/html/content/src/nsTextEditorState.cpp
@@ +639,5 @@
>    NS_DECL_NSISELECTIONLISTENER
>  
>    NS_DECL_NSIDOMEVENTLISTENER
>  
> +  nsresult EditAction();

Nit: please also include "virtual" in the declaration.

@@ +828,5 @@
>  
>    return NS_OK;
>  }
>  
> +// BEGIN EditActionListener

Nit: You can drop this and the END macro, they're pretty much useless!

::: editor/idl/nsIEditor.idl
@@ +25,5 @@
>  interface nsIEditActionListener;
>  interface nsIInlineSpellChecker;
>  interface nsITransferable;
>  
>  [scriptable, uuid(7ad59e28-f3d5-4e14-8ea3-794ad4a86de3)]

Please change the uuid of this interface as you're changing its methods.

@@ +517,5 @@
>     * and document state listeners.
>     */
>  
>    /** add an EditorObserver to the editors list of observers. */
> +  void addEditorObserver(in EditActionListener observer);

Instead of this name, please use "setEditorObserver" which does not suggest that there are multiple observers.

@@ +522,3 @@
>  
>    /** Remove an EditorObserver from the editor's list of observers. */
> +  void removeEditorObserver();

Hmm, I was going to suggest that we can remove these from the nsIEditor interface and move them to nsEditor, but that requires nsIEditor to be builtinclass, otherwise JS and other things can also implement nsIEditor, in which case we don't wanna cast it to nsEditor*.  So this looks good for now.

::: editor/libeditor/base/EditActionListener.h
@@ +9,5 @@
> +class EditActionListener
> +{
> +public:
> +
> +  virtual nsresult EditAction() { return NS_OK; }

Please make this pure virtual.  Also, we don't use the return value, so let's just make it void.  :-)

::: editor/libeditor/base/nsEditor.cpp
@@ +69,5 @@
>  #include "nsIDOMText.h"                 // for nsIDOMText
>  #include "nsIDocument.h"                // for nsIDocument
>  #include "nsIDocumentStateListener.h"   // for nsIDocumentStateListener
>  #include "nsIEditActionListener.h"      // for nsIEditActionListener
> +#include "EditActionListener.h"         // for EditActionListener

Nit: please reorder this so that the include list would still be alphabetical.

@@ -174,5 @@
>   NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mTxnMgr)
>   NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mIMETextRangeList)
>   NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mIMETextNode)
>   NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMARRAY(mActionListeners)
> - NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMARRAY(mEditorObservers)

You should probably point to the new member object using a native ptr macro...

@@ -193,5 @@
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR_AMBIGUOUS(mTxnMgr, nsITransactionManager)
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mIMETextRangeList)
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mIMETextNode)
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMARRAY(mActionListeners)
> - NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMARRAY(mEditorObservers)

Same here.

@@ -445,5 @@
>  
>    // Unregister event listeners
>    RemoveEventListeners();
>    mActionListeners.Clear();
> -  mEditorObservers.Clear();

Please set the observer to null here.

@@ +1766,3 @@
>    NS_ENSURE_TRUE(aObserver, NS_ERROR_NULL_POINTER);
>  
> +  mEditActionListener = aObserver;

Also, MOZ_ASSET that mEditActionListener is null when this is called, to make sure that nobody uses this as an observer list.

::: editor/libeditor/base/nsEditor.h
@@ +53,5 @@
>  class nsIDOMRange;
>  class nsIDocument;
>  class nsIDocumentStateListener;
>  class nsIEditActionListener;
> +class EditActionListener;

This should not be needed, since this file #includes nsIEditor.h.
Attachment #661047 - Flags: feedback?(ehsan) → feedback+
(Assignee)

Comment 6

5 years ago
The only question I have is with the "You should probably point to the new member object using a native ptr macro..." part ...

I really don't understand the cycle collection macros yet. I currently have mEditActionListener defined as a raw pointer, vs nsCOMPtr<> kinda thing, so adding a NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mEditActionListener) doesn't leap out at me.

And if you're thinking I need to use NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_NATIVE somehow then I'm really lost, as example code I scanned is sparse.

Can you explain a little further?
Hmm, you know what?  That pointer is not a strong reference, so I may in fact be wrong, and you may not need to mark that at all in the cycle collection functions.  CCing smaug to confirm this.
Yeah, if it isn't a ref counted pointer, you shouldn't need to tell the cycle collector about it.
(In reply to comment #8)
> Yeah, if it isn't a ref counted pointer, you shouldn't need to tell the cycle
> collector about it.

OK good.  So please ignore my comment about the cycle collector then, Mark!
(Assignee)

Comment 10

5 years ago
Created attachment 661414 [details] [diff] [review]
Patch (v2)

B)

Made the changes, re-tested, push to TRY looks good:
https://tbpl.mozilla.org/?tree=Try&rev=61dd7a5e116f
Attachment #661047 - Attachment is obsolete: true
Attachment #661414 - Flags: review?(ehsan)
Comment on attachment 661414 [details] [diff] [review]
Patch (v2)

Review of attachment 661414 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!  Thanks so much for your patch, Mark!  :-)
Attachment #661414 - Flags: review?(ehsan) → review+
Landed with my first name fixed in the commit message.  :-)

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4a470dfb0b2
Target Milestone: --- → mozilla18
https://hg.mozilla.org/mozilla-central/rev/f4a470dfb0b2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 792979

Updated

5 years ago
Keywords: dev-doc-needed → dev-doc-complete
Guys, *please*, when you change something like that, add me to the cc list...
I'm only one of the largest nsIEditor users around, eh?
For the record, BlueGriffon is/was using it :-/
glazou, do you need nsIEditorObserver?
It is not too late to add it back, if someone provides backout patch.

Also, perhaps you could watch all the editor bugs. There shouldn't be too many bugmails.
(In reply to Olli Pettay [:smaug] from comment #15)

> glazou, do you need nsIEditorObserver?
> It is not too late to add it back, if someone provides backout patch.

I am using it at least once, and that's a pretty major piece of code
to observe everything going on in the editor. I'll investigate more
tomorrow when I'm back at the office (I'm in town right now) and will
keep you posted.

> Also, perhaps you could watch all the editor bugs. There shouldn't be too
> many bugmails.

Yeah. Will do that. But cc:ing me on important editor stuff is not that
difficult, is it?
Not everybody knows that you'd want to be CC'ed. The same way that not everybody knows that
I should be CC'ed to all the DOM Events and Event Handling bugs - so I watch both those components.
Ok, it seems I *could* use the new code but there is something looking
wrong with it... Here's the new API or nsIEditor for the observer:

  void setEditorObserver(in EditActionListener observer);

and if in my JS code, I try to reach it, it's undefined, the only method
available on nsIEditor that remains undefined although it's in the IDL.

Is it because the argument is an EditActionListener defined as a C++
class and not an IDL and in that case made automatically unscriptable?

I *absolutely* need to be able to set that observer from JS code because
the observer is notified from nsPlaintextEditor::Undo and ::Redo...
Ok, I think we should back this out. Ehsan, comments?

Updated

5 years ago
Depends on: 811679
(In reply to Olli Pettay [:smaug] from comment #19)

> Ok, I think we should back this out. Ehsan, comments?

Another possibility that I would be fine with would be to extend
nsIEditActionListener and add DidUndo() DidRedo() stuff there and
call them from the nsPlaintextEditor Undo/Redo methods. But that
could impact all current users of nsIEditActionListener.

Unfortunately, nsIEditorObserver is currently the _only_ way to
be notified an Undo or Redo event occurred in the editor. I tried
other approaches w/O success in the past.
Sorry for the late reply here, Daniel.  Have you tried to see if you can use MutationObserver instead of nsIEditorObserver?  The Thunderbird people managed to use it because they needed to get notified every time that the editable document changed.  If that doesn't work for you, we can look into backing this out, but I'd like to avoid doing that if possible.  :-)

Please ping me on IRC if you want to discuss this.
(In reply to Ehsan Akhgari [:ehsan] from comment #21)
> Sorry for the late reply here, Daniel.  Have you tried to see if you can use
> MutationObserver instead of nsIEditorObserver?  The Thunderbird people
> managed to use it because they needed to get notified every time that the
> editable document changed.  If that doesn't work for you, we can look into
> backing this out, but I'd like to avoid doing that if possible.  :-)
> 
> Please ping me on IRC if you want to discuss this.

I tried that strategy in the past and it has issues for me because it
reports far more than I need and because I need something specific
to Undo and Redo. As I said earlier, I don't *absolutely* need a
backout, but I need a *scriptable* way of being notified an Undo or
Redo occurred and the current code, from this bug, does not allow it.
See what I said in comment 20 above.
Resolution: FIXED → WONTFIX
You need to log in before you can comment on or make changes to this bug.