Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 228 fixed #259

Merged
merged 29 commits into from May 16, 2016
Merged

Issue 228 fixed #259

merged 29 commits into from May 16, 2016

Conversation

jncharon
Copy link
Contributor

@jncharon jncharon commented May 5, 2016

Confirmation dialog upon vault removal

@jncharon
Copy link
Contributor Author

jncharon commented May 5, 2016

At last ! After a lot of struggle with GitHub desktop I managed to create a PR from my local issue-228-fixed branch to upstream:delete-confirmation.
I hope it'll be up to your expectations, let me know if there is something wrong.

@overheadhunter
Copy link
Member

What license applies to the icons?

@jncharon
Copy link
Contributor Author

jncharon commented May 5, 2016

It's the standard icons shipped with JavaFX so they have the same licence as the framework itself

@overheadhunter
Copy link
Member

Is there any way to use them without re-bundling them?

@jncharon
Copy link
Contributor Author

jncharon commented May 5, 2016

They are normally bundled in a javafx-ui jar along the CSS files. We could include this jar in the project but we would have to manage the css priority between the files in the jar and the ones within cryptomator.

@overheadhunter
Copy link
Member

Oh ok I see, css and images have to be in the same jar in order to make -fx-graphic: url("dialog-more-details.png"); work.

@jncharon
Copy link
Contributor Author

jncharon commented May 5, 2016

The icons are in the javafxrt.jar, we should be able to use them to without puting them within cryptomator. How come cryptomatir doesn't use the default CSS ressources ? Because adding the CSS code wasn't enough to have the icons showing up, this is why I rebundled them

@@ -13,6 +13,9 @@ main.directoryList.contextMenu.remove=Remove from list
main.directoryList.contextMenu.changePassword=Change password
main.addDirectory.contextMenu.new=Create new vault
main.addDirectory.contextMenu.open=Open existing vault
main.directoryList.remove.confirmation.title=Vault removal
main.directoryList.remove.confirmation.header=Do you really want to remove this vault ?
main.directoryList.remove.confirmation.content=Every data it contains will be lost.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file doesn't exist any longer in master, as we switched from .properties files to unicode .txt files due to charset issues with non-latin alphabets.
Please merge from master and put the new strings into src/main/resources/localization/en.txt

@jncharon
Copy link
Contributor Author

jncharon commented May 5, 2016

From a licence point of view, isn't a rebundle the same as using the default components ?

@overheadhunter
Copy link
Member

overheadhunter commented May 5, 2016

In our "native" bundles all libraries stay separated, so we can prove to just "use" them. The only exception is the "fat jar" in GitHub Releases, which mixes all classes together. Technically this remixing may cause issues if licenses prohibit such things.

However JavaFX is licensed under the Oracle BCL. I am not sure, to what extent parts of the software can be re-packaged. Depending on the mood of lawyers repackaging might be a "modification" and not just a "use" of provided resources.

Edit: OpenJFX is licensed under GPL terms. This means if we redistribute parts of the code (or in this case images), Cryptomator as a whole becomes GPL-licensed. But we want to stick with the MIT license, as we consider it less restrictive.

@jncharon
Copy link
Contributor Author

jncharon commented May 6, 2016

I merged upstream:master, is the content of the pull request ok now ?

@@ -11,3 +11,33 @@
.classpath
target/
test-output/
.idea/compiler.xml
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just use .idea/

@overheadhunter
Copy link
Member

Can you somehow untrack .idea/* files? The accepted answer to this Stackoverflow question might help.

@jncharon
Copy link
Contributor Author

jncharon commented May 7, 2016

I tried to remove them from within intellij but to no avail. I'll try the solution in your link !

@jncharon
Copy link
Contributor Author

jncharon commented May 8, 2016

Ahem, the git -rm fucked up my project setup, I had to reconfigure a new project within intellij.
Now I've put the .idea directory in another folder so project is not mixed with the sources.
I don't know if this will be reflected in the pull request (as it's a new project) or if I have to close it and create a new one ?

@overheadhunter
Copy link
Member

So far there is no new commit visible in this PR. The last commits I can see are the following ones:

bildschirmfoto 2016-05-08 um 20 08 24

@overheadhunter overheadhunter merged commit dc87dad into cryptomator:delete-confirmation May 16, 2016
@overheadhunter
Copy link
Member

Still need to check license of icons, will discuss this with @MuscleRumble, who has a lot of icons that we bought the license for. If necessary we will change them.

@jncharon
Copy link
Contributor Author

Sounds good :)

@jncharon
Copy link
Contributor Author

Hi Sebastian,

Do you have any plans for a "contributors" page so we could be eligible to the IntelliJ licence for Open Source projects ?

And thanks for the merge by the way !

Best regards
Jean-Noel

Le 16 mai 2016 à 12:54, Sebastian Stenzel notifications@github.com a écrit :

Still need to check license of icons, will discuss this with @MuscleRumble, who has a lot of icons that we bought the license for. If necessary we will change them.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub

@overheadhunter
Copy link
Member

What exactly does the IntelliJ license need us to do? Have we used any IntelliJ material?

Currently we just have the contributors page on GitHub, but we plan to add some pages to the website anyway.

@jncharon
Copy link
Contributor Author

I'm using IntelliJ and they have free licences for open source projects : https://www.jetbrains.com/buy/opensource/?product=idea
They have to check that people are real contributors to issue those licences

@overheadhunter
Copy link
Member

Ok I will see, if we can add a team/contributors page this week.

However I see a problem with the conditions:

Project must not provide commercial services or distribute any paid software versions

As you know, our iOS app is a paid app - don't know if this works...

@jncharon
Copy link
Contributor Author

the ios app in in another project, I hope they won't care.
If we are not eligible I'll use the licence i've got from work but I'd prefer not to mix up things.

@overheadhunter
Copy link
Member

@jncharon added github contributors onto our website: https://cryptomator.org/team/

However we want to emphasize that in our opinion the MIT license is not compatible to the conditions of IntelliJ. As long as you publish source code under the MIT license (otherwise we would reject it) the rules of the MIT license apply.

If your IDE forbids creation of non-copyleft code, don't use it or don't tell me 😉

@jncharon
Copy link
Contributor Author

The iOS app is a separate project and we won't use Idea for it. If I'm not eligible it won't be a big deal, I'll just buy a personal licence !
What are the conditions to be considered as a contributor and be listed on the website ?

@overheadhunter
Copy link
Member

The iOS app is not the problem, but the MIT license doesn't restrict the commercial use of the desktop app either. For example we're planning to offer payed service for the desktop app.

People are considered to be contributors, as soon as they have code merged into the project (i.e. pull requests). Bug reporters are not contributors. We get this data via JSONP directly from the GitHub API. They make the rules ;)

@jncharon
Copy link
Contributor Author

Oh, it's weird I'm not in the list despite my two merged pull requests 😕

@overheadhunter
Copy link
Member

overheadhunter commented May 18, 2016

@jncharon
Copy link
Contributor Author

My bad, the browser didn't display this area until I cleaned the cache. Not it's okay ! 👍

@jncharon jncharon deleted the issue-228-fixed branch May 26, 2016 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants