sghpjuikit / player

Audio player and management application.
22 stars 2 forks source link

Feature/kotlin update #90

Closed sghpjuikit closed 6 years ago

sghpjuikit commented 6 years ago

Note:

sghpjuikit commented 6 years ago

Another thing, I have set jvmPath to be the the local instance, but that seems to crash the remote build. Any ideas?

sghpjuikit commented 6 years ago

I have tried to use contracts for the getItem() issue, but the result is disappointing.

My attempt

@ExperimentalContracts
fun Item.isFileBased(): Boolean {
    contract {
        returns(true) implies (getFile() is File)
    }
    return "file"==uri.scheme
}

I'm getting 'Only references to parameters allowed in contracts' for using getFile() after implies.

End result: this is very experimental as of now and will have to wait for a while longer.

xeruf commented 6 years ago

I have no idea at all about contracts.

The build seems to try to compile before linking the jdk, if you remove the jdk link locally it will probably fail for the first run as well. I will configure gradle so it runs the jdk linking task first.

Btw, please use triple ``` for multi-line code for readability

sghpjuikit commented 6 years ago

Very good job on cleaning up the build file (it is always educating experience for me to read your changes in this file). However, in the future, please refrain from changing the parts that are of my responsibility - those related to the PR. It is a bad taste. I know you opted for efficiency here and you did save me work, but it was still improper.

xeruf commented 6 years ago

I guessed that, but how else should I do that? Should I make a PR into your PR?

sghpjuikit commented 6 years ago

Not at all, simply leave the parts that you requested of me to change for me.

xeruf commented 6 years ago

But the changes were mostly not things worth explaining, just a few small adjustments. These are things that I can see when I'm editing the file but they maybe wouldn't be apparent to you, but you can understand their purpose just by looking at the diff. So either I commit into the branch (it is good practice anyways to always pull before you start working) or open a PR into the feature-branch.

sghpjuikit commented 6 years ago

You made 2 changes that we were specifically talking about, that's all.

sghpjuikit commented 6 years ago

What is the + notation in the version of the dependendies in the build file? To use latest minor version automatically? If that is the case I disagree with using that feature. Automatic dependency pulling is unpredictable and dangerous. Only developer can raise the version.

sghpjuikit commented 6 years ago

I have also finally figured out why my project start up takes 20-30 seconds sometimes. After you updated the build file and the linkJdk task name is printed. It is the linkJdk task!! It always executes for me when run the project! I finally know this drove me absolutely crazy! Ill look into this as well.

xeruf commented 6 years ago

The linkJdk task name is printed? Huh? What's going on?

Yes the + is for automatically using the latest minor version. I thought about whether or not to put it in, but I thought it might be helpful to not have to update them manually all the time. If you disagree, feel free to revert it.

sghpjuikit commented 6 years ago

No, please remove the +. I understand the advantages, but we are opening up a way for (potentially serious) bugs to worm in. Like XStream update screwing up widget serialization.

I'm not sure about linkJdk, lets handle that in separate issue (if it even is an issue to begin with). It is not running always for me (well it is, but it runs instantly). So far I have only observed it to run a long time once, but that should never happen at all, since my jdk is already linked.

xeruf commented 6 years ago

+ has been removed again

I found the issue with linkJdk: Because of compilation failures I needed to make compileKotlin depend on it, but I didn't properly configure it to communicate to Gradle when it is up to date and does not need to run, so it should be fixed now.

sghpjuikit commented 6 years ago

I understand what you did with linkJdk task (nice), but I dont think it should impact my performance, because it doesnt matter whether gradle runs the isUpToDate check or the task itself - the linking should never occur on my machine, yet it seems it sometimes did. I will observe the behavior with your solution.