tesis-dynaware / graph-editor

Eclipse Public License 1.0
132 stars 42 forks source link

CompoundCommand.appendAndExecute() does not trigger a re-layout #18

Closed eckig closed 9 years ago

eckig commented 9 years ago

Somewhere you suggested to use CompoundCommand.appendAndExecute() in GraphEditor.setOnConnectionCreated() etc.

If I use these hooks to just set some internals (e.g. an ID) it works just fine, but if I tweak some settings affecting the display of the graph nothing happens and I have to do a manual GraphEditor.reload() which feels not quite right?

PS: A new Command submitted with execute() triggers a re-layout, just the appendAndExecute() does not.

rmfisher commented 9 years ago

Hrm is that right? I'll look into this. I thought the CommandStackListener would also fire after appendAndExecute, but perhaps not if there's no new command.

eckig commented 9 years ago

I am currently writing an EContentAdapter for the GModel registering every change made to the GModel. This would solve this problem as the adapter does not depend on changes to the CommandStack. Should I do a pull request for a working solution?

On top of that, we would get rid of the "repaint everything" method. Just repaint the things that got changed..?

rmfisher commented 9 years ago

An EContentAdapter will be fired multiple times if multiple attributes are changed at once (e.g. you resize a node and the width, height, x, and y values can all change). Also I recall having problems where the EContentAdapter did not always fire in some cases.

Note that the initializeAll method in GraphEditorController (I assume this is what you mean by repaint everything) only adds or removes the deltas from the scene graph, so it is actually quite fast. I'd only accept a pull request if there was a measurable performance improvement.

eckig commented 9 years ago

I didn't know that you already tried the EContentAdapter. Was just thinking, that this could be a solution for the appendAndExecute().

eckig commented 9 years ago

Is it fixed?

rmfisher commented 9 years ago

I guess we can live with graphEditor.reload(), unless there is a more concrete plan for what to change.