Cleanup Script Editor

[Squareys]

Hello everybody,

as part of imagej/imagej-ui-swing#40, I cleaned up the existing script editor code, extracted some classes for readability and added a bunch of javadoc. This involved code formatting, which I tried to keep in separate commits. Everything is done in small and (hopefully) clear steps.

In general this is all refactoring and should not result in any visible changes for the user, but does break the API every now and then (changing member access to private and providing a getter for example).

Greetings, Squareys

[ctrueden]

I'm curious about the rationale for 5ee745f. Would be good to write into the commit message why you are making the change—e.g., does it avoid a race condition or something? If so, would be good to paste the relevant stack trace(s) into the commit message, so the problem being solved is more clear.

[Squareys]

I'm curious about the rationale for 5ee745f. Would be good to write into the commit message why you are making the change—e.g., does it avoid a race condition or something? If so, would be good to paste the relevant stack trace(s) into the commit message, so the problem being solved is more clear.

Actually I just thought it might be better style. As I mentioned, creating an Editor in a Scripting Window (TextEditor class) which is null does not actually make sense. Especially, since there is no way to move the editor into a later.

I'm having the feeling you might have missed that that code was synchronized before via a synchronized method. I just inlined it, since it was merely one method call. (I therefore did not change synchronization. But the inlining in the end was pretty unnecessary, because of the synchronized blocks.)

[ctrueden]

OK, I see what you mean.

I disagree that inlining is better style. Quite the contrary. If you have code in multiple places that does the same thing, it should be a dedicated method. That is the DRY principle: "Don't Repeat Yourself."

So I definitely think the setTitle method should not be removed.

[Squareys]

What if the setTitle does frame.setTitle()? It cannot be considered good stlye to have a method for that. Since it was synchronized, that is different though. I just realized later, the synchronized stamements were ammended to that commit.

(I never meant to say inlining is good style in general.)

[ctrueden]

The setTitle method performs the following actions:

1. Synchronizes, so it cannot be called concurrently.
2. Verifies that the frame is not null.
3. Sets the frame's title if so.

That is three separate steps, which are always performed together like that. So it should be a method.

I understand that it bothers you that a method called setTitle also calls another method called setTitle, but that does not change the fact that it is more DRY to do it this way. If you want to feel better, you could rename the setTitle method to setFrameTitle (or even setFrameTitleAsNeeded or something even more Germanverbose :wink:) since it would be more accurate declaration of that method's purpose.

[Squareys]

@ctrueden I worked hard to decouple EditorPane from TextEditor, which means those setTitle() calls moved into TextEditor now anyway. (https://github.com/Squareys/imagej-ui-swing/commit/58b3cb17ac9d7aa5f5b9e4ecb50586b95f19994b)

The decoupling is very important for us to be able to reuse EditorPane in the KNIP ScirptEditor, as you originally suggested.

As for the discussion we had about inlining: We were talking past each other. If we ever meet in person on a hackathon again, I'd be glad to explain.

I was not able to address imagej/scijava-issue-shuttle#1 just yet. I will do so in a separate pullrequest. As soon as you merge this, I can file a pullrequest for the codeassist/autocomplete.

Greetings, Squareys.

[Squareys]

@ctrueden I merged the "Remove obsolete TODO" commit with the one where it was fixed (and created).

The synchronization issue turned obsolete after I refactored to decouple EditorPane from TextEditor. As I mentioned above, If you are still curious about why I thought inlining was a good idea, we may discuss that in person at some hackathon. As I said: We were talking past each other.

All together, toggling bookmarks doesn't seem to work, but it turns out, they don't work on master either. You might want to double check, though.

All in all, This is ready for merge, and I already finished the autocompletion/codeassist, so I'd be glad, if this PR could be concluded soon, since that will depend on this one.

Greetings, Squareys.

[hinerm]

Thanks @Squareys All of these changes are great, and it makes my heart especially happy when I see new javadoc on previously undocumented code. It's much appreciated.

[ctrueden]

:+1:! And just in time for the ImageJ conference too!