AbstractPushToApplication

[oscargus]

Here's my go at an AbstractPushToApplication class. So far I've implemented PushToTexmaker.java using the new class. See the last commit, the earlier commit is primarily refactoring all these files to external.push.

Comments so far? I thought it might be a better idea to get some input on the construction of the abstract class before I continue converting the rest.

The abstract class will work "out of the box" if there's just a single path to be set. In that case one will have to define the four methods in PushToTexmaker: name, icon, command line, and an init, primarily meant to get the preference key (maybe better to have a getPreferenceKey()? I was originally thinking that maybe more preference values could be read in here in case of a more complicated configuration dialogue.)

(I know that I forgot to run auto-format for some files and also noted some other things that can be slightly improved, like in ExternalTab, but those will be sorted out eventually. Comments on AbstractPushToApplication.java and PushToTexmaker.java primarily. :-))

[simonharrer]

undefined?

[oscargus]

Yes, would make sense.

[simonharrer]

From what I see when looking at all the classes:

• the all make heavy use of Swing for the preferences plus the logic of interfacing with the external applications. To reduce complexity, this could be separated. Maybe just use static inner classes for the external tool wrapper.
• the push classes differ in their number of preferences used (most only use a path, but emacs use emasPath:String, additionalParams:String, useEmasc23:boolean, and vim uses both a vimPath:String and a vimServer:String).
• pushEntries and operationCompleted communicate via two boolean variables. These two methods should be grouped together in a separate class as all other methods have nothing to do with them.
• you need to be aware that the OpenOfficePanel also implements the PushToApplication interface. And this class is large (> 1000 LoC).

[oscargus]

• Not sure if I fully understand the first bullet.
• Yes. In the yet to be pushed update, I used the superclass method for creating the JPanel and path textbox and added other fields as needed.
• I've merged part of the functionality. The main problem I see is that there are a few different ways to check if it actually was successful, but I can not really say that I totally understand all the details. Now, I've introduced these methods in the abstract class and where applicable I only ask for the command string and check if the PushToClass provides its own error messages.
• Yes, that is something which I haven't plan on touching at this stage. Therefore, I try not to touch the interface at all. This may limit things a bit, but I'd still say that I get the amount of duplicated code down quite a bit and implementing a new (simple) PushTo would be even simpler than before without any cloning of code.

I'll push during the evening if things goes as planned.

[simonharrer]

could we have a logging statement instead of a stackTrace?

[oscargus]

Yes!

[simonharrer]

Two larger suggestions:

• Create a class PushToApplications that contains the applications variable plus the init code currently located in PushToApplicationButton of all PushToApplication instances. Also move the PushToApplicationComparator class to the newly created PushToApplications class.
• I would not make the getCouldNotCall and getCouldNotConnect methods. If the subclass wants other error messages, it has to override the operationCompleted method. This should also simplify things a little bit.

[oscargus]

For the second bullet: consider PushToLyx, would it be better to basically copy the operationCompleted method and just provide different messages? (The reason that PushToLyx needs different messages is that it communicates through a pipe/file, so the standard messages does not make sense, while the basic error behavior is quite similar.)

For the first bullet: let me see if I understand it. I'll get back. :-)

[oscargus]

And thanks for the feedback! 👍

[oscargus]

I think I understood the first bullet and have updated according to your suggestions (except the ones I didn't agree with, but let's see if you insist).

I realized a few more things that must be fixed:

• ChangeLog
• Copyright in new class
• Two remaining printStackTrace to be removed

Working on it...

[simonharrer]

just remove this comment

[oscargus]

Bah! Talk about copy-paste-programming...

[simonharrer]

This is what I meant with the other bullet point 73a9ca1

[oscargus]

OK! Makes sense. The reason I choose my approach was to always get some output in the status line. In your approach, there will "only" be a dialog in some cases. Not that it matters, so I'm happy to go with your approach.

[simonharrer]

Just for completeness: in your approach there may have been the case that at least one output may be shown (couldNotConnect could "fire" twice, once in the subclass and then in the superclass) whereas in my approach, exactly one output will be shown.

[oscargus]

Sort of. I admit that there could be some guidelines how to write a new PushToXXX (I was thinking about that). From my perspective (/in my version), the only reason for a class to have its own operationCompleted is that it wants to do something else than just showing something in the status bar. So depending on what you want to obtain:

• Change status bar message
  • Use getCouldNotCall etc, no need for a new operationComplete
• Show a dialog
  • Use operationComplete, but the status bar will still be changed without the need for replicating that part of the code

Now, one can question if it is worthwhile to both show a dialog and change the status bar. It may be that one would like to a have a more uniform behavior for the (simple) PushToXXX (the OpenOffice one kept out of the discussion). Now, some just executes the command and if the system does not return an exception it "worked", others parse the result (or rather see if the call did return some text). Some show a dialog, some just a status bar message. Making this more uniform would change the design a bit. But I wasn't comfortable in deciding on the actual behavior of the classes (in a major way).

[simonharrer]

Okay, now I get it.

Nevertheless, I would make it simpler first with lesser methods. If these methods are overridden in more classes, we can still extract them.

Can you please rebase your changes on the current master branch? Then I can merge this in and add my small change as well.

Thank you again for your effort in improving the code quality of JabRef.

[oscargus]

OK! Just to avoid issues similar to those in my other PR: if I just do a git rebase masterand then push it, everything should be fine and not fifty or so extra commits? Thought I had a grip of things, but apparently not.

[simonharrer]

Yes, sounds correct. :-)

[oscargus]

No, that didn't work as expected. No magic involved this time either. Just git rebase master. When I tried to push, it failed as I had to do a pull first. That didn't look like it contained anything (see the final commit). Then pushed and now this...

[simonharrer]

Ah, you have to do a force push instead of a pull.

[oscargus]

OK, so now I reset, rebase and force push?

[simonharrer]

Yes. May the force be with you.

[oscargus]

OK, now I think it is OK. At least the force-pushed version and the previous pull-push version are identical. Even though I mess these things up regularly, at least I get some practice in restoring things...

[oscargus]

OK! I'll make a new attempt tonight. May contact you one way or the other
to try and synchronize the push.

[oscargus]

@simonharrer Now I eventually got around to update the branch. Please merge ASAP. ;-)

[koppor]

I have the issue that the application is always reset to "LyX". Does anyone else experience the same issue? - Furthermore, I'd like to have the default application when JabRef is freshly installed, to be TeXstudio. 😇

[oscargus]

I've noted that it happens, yes, and I do not think it happened earlier. As far as I know I haven't changed anything related to that though...