RFR: JMC-6555 Convert JOverflow plugin to SWT

[Arvin Kangcheng Xu]

Thank you all for reviewing my code.

On Mon, 26 Aug 2019 at 09:30, Mario Torre <neugens at redhat.com> wrote:
>
> On Mon, Aug 26, 2019 at 3:24 PM Mario Torre <neugens at redhat.com> wrote:
> >
> >
> >
> > On Mon, Aug 26, 2019 at 3:16 PM Jie Kang <jkang at redhat.com> wrote:
> >>
> >>
> >> The thread safety of this construct is concerning. Is it expected to
> >> only be called by a single thread? If yes, I'd appreciate a comment
> >> specifying that it is not thread-safe, but it is okay because no more
> >> than one thread calls this. If no, then I think a lock would be more
> >> appropriate than a boolean.
> >>
> >> Also there is a guard in updateModel() but no guard in reset(); is
> >> that intentional? Can things go awry if reset occurs while updateModel
> >> is executing? I guess these kinds of questions need only be considered
> >> if there is multi-threaded access...
> >
> >
> > I'm not an expert of the Eclipse threading model, but I would be very surprised if the UI was updated outside the rendering thread, and afaik SWT does not allow access from multiple thread on its component hierarchy, so I would rather add a comment if this code was meant to be accessed by multiple threads since that would be the exception.

Yes, you are right Mario. updateModel() is only supposed to be called
from the SWT event thread. Any access from outside this thread will
result in a SWTException by its design.

> P.S.
>
> In the case where the model is updated externally of the event thread,
> you will likely need in the calling code some syncing libraries like
> Display.asyncExec (or whatever Eclipse version of invokeLater is).
> Indeed, the mIsUpdatingModel throws me off too, since it seems to
> imply the view is being update concurrently.

While updateModel() is not accessed outside the thread, it's possible
to be called multiple times, before the previous calls finish, from
the SWT event thread. From what I understand, SWT operates on some
kind of event loop design. In the process of updating the UI, it's
possible for some SWT listeners to be invoked and call updateModel()
again. (eg. SelectionListener listener is called when the table
content is updated.) Without this mIsUpdatingModel, it can easily
result in an endless recursion. However, I do agree that guarding with
a boolean is not the most elegant solution. Please let me know if
there is a better approach coming across your mind.

Thank you!
Arvin

> Cheers,
> Mario
>
> --
> Mario Torre
> Associate Manager, Software Engineering
> Red Hat GmbH <https://www.redhat.com>
> 9704 A60C B4BE A8B8 0F30  9205 5D7E 4952 3F65 7898