Fix open folder action in Windows not working correctly... #1093
Conversation
| @@ -52,6 +52,7 @@ to [sourceforge feature requests](https://sourceforge.net/p/jabref/features/) by | |||
| - [#1023](https://github.com/JabRef/jabref/issues/492) ArXiv fetcher now also fetches based on eprint id | |||
|
|
|||
| ### Fixed | |||
| - Fixed "Open-folder" action in Windows not opening tge correct folder when folder path contained whitespaces | |||
There was a problem hiding this comment.
The Changelog entry is NOT needed. JabRef worked fine in 3.2. Please remove the line.
tobiasdiez
added
the
status: ready-for-review
|
LGTM 👍 |
|
I further refactored the OS classes to remove redundant code. |
| @@ -156,6 +158,7 @@ public static boolean openExternalFileAnyFormat(final MetaData metaData, String | |||
| private static void openExternalFilePlatformIndependent(Optional<ExternalFileType> fileType, String filePath) | |||
| throws IOException { | |||
| if (fileType.isPresent()) { | |||
|
|
|||
|
I liked the fix before. I don't like the refactoring so far. For me it would have been a better solution to first merge the fix and later discuss the changes in the logic and the general architecture in a separate PR. |
|
@stefan-kolb Well. That would be no problem. as I have split it up in 2 commits. |
| import net.sf.jabref.external.ExternalFileType; | ||
| import net.sf.jabref.external.ExternalFileTypes; | ||
|
|
||
| public abstract class NativeDesktop { |
There was a problem hiding this comment.
Can you use an interface with default methods instead?
There was a problem hiding this comment.
I don't really like having an interface with default methods. I chose the abstract class because from my point of view a class hierarchy better represents the is-a hierarchy in this case. e.g. a OSX(Desktop) is a Native Desktop. An Interface is more a kind of Role-based thing and in this case not the best way.
There was a problem hiding this comment.
In that case, the concrete method hasRegisteredApplication is never overridden by their sub classes, and could be made static. From my point of view, It should be a method of the class ExternalFileType (without the Optional parameter of course).
The other concrete method is also never overridden and has only two very simple lines of code. It is only required in two places in the JabRefDesktop class which are only a few lines apart in the same method. Hence, I would move this method to the calling side, as it does not really help.
With this said, I would convert the abstract class back to an interface without default methods which can be handled more easily (favor composition over inheritance).
|
I looked a bit further around and saw some more relating things to ExternalFileType which could be improved. However, that would take some more time to fix and be beyond this scope. Will do that in a different PR. |
when path contains whitepsaces
|
Rebased the changes. |
|
👍 LGTM |
|
LGTM 👍 |
when direcory path contained whitespaces
Use Path-class and methods
"& Symbol" does not cause problems on Windows
Use Path.getParent instead of ugly substring-hack