archi-contribs / specialization-plugin

Plugin for Archi to specialize concepts (figure, icon...)
Other
29 stars 2 forks source link

Magic Connector not working #25

Closed Phillipus closed 5 years ago

Phillipus commented 5 years ago

Hi Herve, with this plugin installed the Magic Cursor is not working as it should do when clicking from a concept to white space.

See https://forum.archimatetool.com/index.php?topic=485.0

Windows/Mac/Linux.

Phillipus commented 5 years ago

Also, when a diagram object has a (large) image as an icon, when dragging the magic connector from the object there seems to be a UI lag. Is there something running in the background on the UI thread? Or perhaps a mouse listener? Or Display#readAndDispatch() being called somewhere?

rchevallier commented 5 years ago

Confirm behaviour issue on my computers with the plugin installed

Phillipus commented 5 years ago

@herve91 Herve, I checked your code. It seems to be an easy fix.

A NPE is being thrown in your code when the Magic Connector tries to display its popup menu. The menu will get images from the UIProvider which can have a null EObject instance at this point.

But SpecializationPlugin.java line 326 (getFullName() method) should check for null obj. You could also check for other possible null values in case there are more.

Phillipus commented 5 years ago

A UIProvider is really providing UI info for an EClass, so does not always have an EObject instance field. When it does, it's because the instance determines the icon or default size which can be different for each (actually there are only two cases of this - Interface type figures for different default size, and the icon for a diagram reference figure)

In fact, you should not rely on the instance field being there at all and you should not use it, as it is not API. You should instead use the EClass of the UIProvider as a key for the icon.

herve91 commented 5 years ago

Thanks a lot for pointing this out.

I'll fix it ASAP ...

Phillipus commented 5 years ago

Rather than just fixing the NPE, if you could make it so that UIProvider#instance is not accessed. Better to use the EClass as a key to get icons.

The problem you will have is with SpecializationPlugin.mustReplaceIcon(this.instance)

The instance field in a UI provider is not guaranteed to be the instance you think it is. It can change at any time. In the future this value may not be there at all.

Phillipus commented 5 years ago

The problem you will have is with SpecializationPlugin.mustReplaceIcon(this.instance)

In your usage of that, you are trying to determine the context for the instance - whether it is in a folder, etc. I understand what you are trying to do, but the instance field shouldn't really be used for that. But I can't think of another way to get the instance in your code, as you are over-riding the UIProvider. So maybe best to just fix the NPE and one day there may be a better system for plugging into the UI via a different extension point.

herve91 commented 5 years ago

All right. If you think of a better way to achieve this, please do not hesitate :)

Phillipus commented 5 years ago

Ideally, we need a plugin extension point so that plugins can provide information such as icon, size, color, label, figures etc for different contexts in response to the main Archi code's UIProviders. But that's not going to happen just yet.

herve91 commented 5 years ago

I'm puzzled because I fail to reproduce the issue on Windows with images 1920x1200. May be very specific to MAC :(

image

Anyway, checking for null values is always a good idea.

Phillipus commented 5 years ago

Not specific to Mac.

  1. Select Magic Connector
  2. Click on Element box
  3. Click on white space on diagram (not another element)

The menu should appear but doesn't.

herve91 commented 5 years ago

Oh, never used this functionality ... So it fails also on Windows ...

Anyway, the fix is ready and I'll sbmit it today.

Thanks again :-)

Phillipus commented 5 years ago

It's not OS specific. As I said, UIProvider#instance can be null as a UIProvider is providing UI information for a class rather than an instance. In future versions of Archi instance will be removed.

herve91 commented 5 years ago

Yes, I got it ...

herve91 commented 5 years ago

Fixed in release 1.0.11