Desktop integration for macos #288

vnashkev · 2020-07-06T13:46:48Z

Application bundle is installed in ~/Applications and not in /Applications folder. Generally, not all users have permissions to install into /Applications.
Application start script [start.sh] does not refer [OpenWebstart] installation but simply calls open with jnlp-url allowing operating system to decide how to open given url. This keeps app-bundle working even if OpenWebStart installation folder has changed. It will work with any available jnlp-url-handler for example with oracle webstart.
Application bundle is installed in ~/.cache/applications.
Application menu link is created in ~/Application folder and is just a symbolic link to app-bundle from ~/.cache/application folder.
Desktop link is just a symbolic link to app-bundle from ~/.cache/applications folder.

Start scripts tries to open jnlp-url without explicit reference to openwebstart.

App-bundle is created in ~/.cache/applications. Corresponding symbolic link is created in ~/Applications

sclassen · 2020-07-06T14:17:34Z

Will this fix #256 ?

hendrikebbers

Can you reformate the code? Will do a more detailed review later.

hendrikebbers · 2020-07-06T14:27:45Z

openwebstart/src/test/java/com/openwebstart/os/mac/AppFactoryTest.java

+	public void testCreateDesktopLink() 
+		throws Exception
+	{
+		if ( !OperationSystem.getLocalSystem().equals(OperationSystem.MAC64) )


Please check https://www.baeldung.com/junit-5-conditional-test-execution#operating-system-conditions for OS specific tests with JUnit 5

hendrikebbers · 2020-07-06T14:28:03Z

openwebstart/src/test/java/com/openwebstart/os/mac/AppFactoryTest.java

+		}
+
+		FileUtils.recursiveDelete(new File(userHome), new File("/tmp"));
+		assert new File(userHome).exists()==false;


Please use Junit Assert

hendrikebbers · 2020-07-06T14:28:21Z

openwebstart/src/test/java/com/openwebstart/os/mac/AppFactoryTest.java

+	    	System.setProperty(JavaSystemPropertiesConstants.USER_HOME, userHome);
+	    	assert userHome.equals(JavaSystemProperties.getUserHome());
+	    	updateCacheHome(userCache);
+	    	assert userCache.equals(FilesystemConfiguration.getCacheHome());


Please use Junit Assert

openwebstart/src/main/java/com/openwebstart/os/mac/MacEntryFactory.java

vnashkev · 2020-07-06T14:39:22Z

Will this fix #256 ?

Yes, this fixes #256

hendrikebbers · 2020-07-06T14:40:17Z

@vnashkev are you familiar with Pull Requests?

vnashkev · 2020-07-06T14:44:45Z

@vnashkev are you familiar with Pull Requests?

sorry, not at all

hendrikebbers · 2020-07-06T14:45:12Z

No problem :) If you have any questions just ask them and we will try to help you ;)

vnashkev · 2020-07-06T14:45:57Z

ok, do you have any guideline for source code format?

hendrikebbers · 2020-07-06T14:46:29Z

I have seen that you did the Pull Request based on a master branch of your fork. While this is ok for us it will be much easier for you to create a branch in your fork and create the Pull request based on that branch instead of the master branch.

hendrikebbers · 2020-07-06T14:47:57Z

Anything that you change in your branch (currently the master branch) before this PR is closed will be reflected in this PR. You can do improvements based on discussions on the master branch of you fork, commit and push it. Once this is done the change should be in this PR, too.

hendrikebbers · 2020-07-06T14:48:59Z

I would say that we are using the default format that comes with IntelliJ / Eclipse. Maybe you can just check to auto format the new and mutated classes by your IDE?

vnashkev · 2020-07-06T14:52:49Z

I have just applied default Eclipse format for AppFactoryTest. Is it ok?

hendrikebbers · 2020-07-06T15:03:11Z

YES :) Let's do it for all the files

vnashkev · 2020-07-06T15:09:24Z

formatting & assert done

hendrikebbers · 2020-07-06T15:15:28Z

ok, as a next step we should refactor the tests:

Please replace all assert calls with the JUnit Assert
As already mentioned we can define the tests to be MacOnly. Be doing so we can remove the initial checks in the methods.
By using a @BeforeEach you can remove the restoreOrigins in each method.

hendrikebbers · 2020-07-06T15:05:48Z

openwebstart/src/test/java/com/openwebstart/os/mac/AppFactoryTest.java

+				Files.delete(link);
+			}
+		} finally {
+			restoreOrigins();


We should add restoreOrigins(); in a method that is called before each test. See https://junit.org/junit5/docs/5.0.2/api/org/junit/jupiter/api/BeforeEach.html

For each test i change user.home system property & update FilesystemConfiguration.cacheHome via reflection. This could be moved into @beforeeach. This change may have an effect of other tests outside of AppFactoryTests. I would suggest to annotate restoreOrigins with @afterall.

hendrikebbers

Some additional changes

hendrikebbers · 2020-07-06T18:11:31Z

openwebstart/src/main/java/com/openwebstart/os/ScriptFactory.java

+		final URL srcUrl = jnlpFile.getSourceLocation();
+		final String schema = srcUrl.getProtocol();
+		final String url;
+		if ("http".equalsIgnoreCase(schema) || "https".equalsIgnoreCase(schema)) {


please externalise "http", "https" and "jnlp" as constants. I assume that we already have defined them in a global constant class / interface but can not check at the moment.

openwebstart/src/main/java/com/openwebstart/os/ScriptFactory.java

hendrikebbers · 2020-07-06T18:15:27Z

openwebstart/src/main/java/com/openwebstart/os/mac/AppFactory.java

+    private final static Path ensureUserApplicationFolder()
+    {
+    	final String userHome = JavaSystemProperties.getUserHome();
+    	final File appFolder = new File( new File(userHome), "Applications");


"Applications" <- CONST

hendrikebbers · 2020-07-06T18:15:32Z

openwebstart/src/main/java/com/openwebstart/os/mac/AppFactory.java

+    }
+
+	private final static Path ensureUserApplicationCacheFolder() {
+		final Path appcache = Paths.get(FilesystemConfiguration.getCacheHome(), "applications");


"Applications" <- CONST

hendrikebbers · 2020-07-06T18:16:18Z

openwebstart/src/main/java/com/openwebstart/os/mac/AppFactory.java

+					final Path linkRealPath = link.toRealPath();
+					return cache.toRealPath().equals(linkRealPath);
+				} catch (final Exception e) {
+					/* ignore this error */


We should at least log this?

hendrikebbers · 2020-07-06T18:16:38Z

openwebstart/src/main/java/com/openwebstart/os/mac/AppFactory.java

+
+    private final static Path getDesktopLink(final String appname)
+    {
+    	return Paths.get(JavaSystemProperties.getUserHome(), "Desktop", appname + APP_EXTENSION );


"Desktop" <- Const

hendrikebbers · 2020-07-06T18:19:10Z

openwebstart/src/main/java/com/openwebstart/os/mac/MacEntryFactory.java

-        return false;
+    public boolean supportsDesktopEntry() 
+    {
+        return true;


Let's switch this back to false. By doing so we can test the behaviour internally and activate the support with a Pull Request in near future :)

hendrikebbers · 2020-07-06T18:22:14Z

@vnashkev we are on a very good way here :) I really like this PR and it will provide much better native support for MacOS. Great work! I would love to deactivate the feature by default with this PR. By doing so we can test it internally and activate it once we have checked that everything is working.

openwebstart/src/test/java/com/openwebstart/os/mac/AppFactoryTest.java

hendrikebbers · 2020-07-08T09:24:09Z

openwebstart/src/main/java/com/openwebstart/os/mac/MacEntryFactory.java

@@ -25,29 +25,38 @@
 public class MacEntryFactory implements MenuAndDesktopEntriesFactory {

    @Override
-    public boolean supportsDesktopEntry() {
+    public boolean supportsDesktopEntry() 
+    {


@vnashkev Looks like the format is broken again ;)

hendrikebbers · 2020-07-08T09:25:43Z

@vnashkev this PR looks very good now :) Only the format in 1 file is broken again. Can you fix this? Once this is done I will be more than happy to merge this PR in OpenWebStart. Thank you very much for this good work!

hendrikebbers · 2020-07-16T07:37:00Z

@vnashkev merged :) Thank you very much for your participation. The fix will be part of the next OWS release

vnashkev · 2020-07-16T11:19:06Z

Thank you.16 июл. 2020 г. 9:37 пользователь Hendrik Ebbers <[email protected]> написал: @vnashkev merged :) Thank you very much for your participation. The fix will be part of the next OWS release —You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or unsubscribe.

vnashkev added 4 commits July 6, 2020 13:35

.gitignore for eclipse

e0ba273

ScriptFactory : start script for JNLP-applicaion under macos.

242b8eb

Start scripts tries to open jnlp-url without explicit reference to openwebstart.

AppFactory (macos) : jnlp-app is installed in user's home folder.

e6ee799

App-bundle is created in ~/.cache/applications. Corresponding symbolic link is created in ~/Applications

desktop link support for macos

d3b4a44

hendrikebbers reviewed Jul 6, 2020

View reviewed changes

sclassen linked an issue Jul 6, 2020 that may be closed by this pull request

OSX Shortcut never creates #256

Closed

AppFactoryTest : formatting

9caa288

formatting

2f111a3

assertTrue instead of assert

b9f95ef

hendrikebbers reviewed Jul 6, 2020

View reviewed changes

vnashkev added 4 commits July 6, 2020 17:23

last assert has been replaced with assertTrue

3460880

AppFatoryTest : using @EnabledOnOs(OS.MAC)

fa52144

using @beforeeach & @afterall

07954df

@afterall must be static

c1d8c2a

hendrikebbers reviewed Jul 6, 2020

View reviewed changes

openwebstart/src/test/java/com/openwebstart/os/mac/AppFactoryTest.java Show resolved Hide resolved

vitali added 6 commits July 7, 2020 07:46

constants for "http", "https" and "jnlp"

07e5449

ScriptFactory.createStartScriptForMac has been removed

1d7f498

constants for "Applications", "applications" and "Desktop"

6a2e2d1

log verification exception, if any

947a1a6

creation of desktop link is deactivated by default

79bfae2

cleanup for AppFactoryTest

087de53

hendrikebbers reviewed Jul 8, 2020

View reviewed changes

MacEntryFactory : format

8b1cb3c

hendrikebbers merged commit 42e4889 into karakun:master Jul 16, 2020

hendrikebbers mentioned this pull request Jul 16, 2020

Native Mac App integration #295

Merged

hendrikebbers added this to the 1.2.0 milestone Aug 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Desktop integration for macos #288

Desktop integration for macos #288

vnashkev commented Jul 6, 2020

sclassen commented Jul 6, 2020

hendrikebbers left a comment

hendrikebbers Jul 6, 2020

hendrikebbers Jul 6, 2020

hendrikebbers Jul 6, 2020

vnashkev commented Jul 6, 2020

hendrikebbers commented Jul 6, 2020

vnashkev commented Jul 6, 2020

hendrikebbers commented Jul 6, 2020

vnashkev commented Jul 6, 2020

hendrikebbers commented Jul 6, 2020

hendrikebbers commented Jul 6, 2020

hendrikebbers commented Jul 6, 2020

vnashkev commented Jul 6, 2020

hendrikebbers commented Jul 6, 2020

vnashkev commented Jul 6, 2020

hendrikebbers commented Jul 6, 2020

hendrikebbers Jul 6, 2020

vnashkev Jul 6, 2020

hendrikebbers left a comment

hendrikebbers Jul 6, 2020

hendrikebbers Jul 6, 2020

hendrikebbers Jul 6, 2020

hendrikebbers Jul 6, 2020

hendrikebbers Jul 6, 2020

hendrikebbers Jul 6, 2020

hendrikebbers commented Jul 6, 2020

hendrikebbers Jul 8, 2020

hendrikebbers commented Jul 8, 2020

hendrikebbers commented Jul 16, 2020

vnashkev commented Jul 16, 2020 via email

Desktop integration for macos #288

Desktop integration for macos #288

Conversation

vnashkev commented Jul 6, 2020

sclassen commented Jul 6, 2020

hendrikebbers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vnashkev commented Jul 6, 2020

hendrikebbers commented Jul 6, 2020

vnashkev commented Jul 6, 2020

hendrikebbers commented Jul 6, 2020

vnashkev commented Jul 6, 2020

hendrikebbers commented Jul 6, 2020

hendrikebbers commented Jul 6, 2020

hendrikebbers commented Jul 6, 2020

vnashkev commented Jul 6, 2020

hendrikebbers commented Jul 6, 2020

vnashkev commented Jul 6, 2020

hendrikebbers commented Jul 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hendrikebbers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hendrikebbers commented Jul 6, 2020

Choose a reason for hiding this comment

hendrikebbers commented Jul 8, 2020

hendrikebbers commented Jul 16, 2020

vnashkev commented Jul 16, 2020 via email