Skip to content
This repository has been archived by the owner on Dec 12, 2020. It is now read-only.

player: Fix Export Current Track and add format option #126

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

defvs
Copy link
Collaborator

@defvs defvs commented Nov 9, 2020

Really strange; Java on Windows doesn't clear the file before writing to it. You have to do it manually.

@defvs defvs self-assigned this Nov 9, 2020
@defvs defvs requested a review from xeruf November 9, 2020 18:10
build.gradle.kts Outdated
@@ -169,3 +170,11 @@ tasks {

println("Java version: ${System.getProperty("java.version")}")
println("Version: $version")
val compileKotlin: KotlinCompile by tasks
compileKotlin.kotlinOptions {
jvmTarget = "1.8"
Copy link
Owner

@xeruf xeruf Nov 10, 2020

Choose a reason for hiding this comment

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

tasks.with<KotlinCompile>...
JavaVersion.VERSION_1_8.toString()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No idea what that piece of code is. My IDE apparently added it from nowhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reverted in 32bff7a

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

and in 4051b69 added back newline

Comment on lines 173 to 178
Files.newBufferedWriter(Settings.PLAYEREXPORTFILE(),
StandardOpenOption.TRUNCATE_EXISTING,
StandardOpenOption.WRITE,
StandardOpenOption.CREATE
).close()
Files.write(Settings.PLAYEREXPORTFILE(), track.toString().toByteArray())
Copy link
Owner

Choose a reason for hiding this comment

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

Have you tried kotlin's inbuild file writing? Also, I think you can pass the StandardOpenOption directly to Files.write

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Doesn't work on Windows. Seems to be an bug within JVM. I've tried passing StandardOpenOption.TRUNCATE_EXISTING which should in theory rewrite the file completely but it simply writes over it, so writing smaller chunk simply writes one character over the old one.

Default options of Files.write should be TRUNCATE_EXISTING, WRITE, CREATE tho. Don't know why I forcefully put them in the bufferedwriter and not the write.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 9a4e5f5

@@ -114,7 +114,11 @@ object Player: FadingHBox(true, targetHeight = 25) {
fun reset() {
fadeOut()
if(!Files.isDirectory(Settings.PLAYEREXPORTFILE())) {
Files.write(Settings.PLAYEREXPORTFILE(), arrayListOf(""), StandardOpenOption.CREATE)
Files.newBufferedWriter(Settings.PLAYEREXPORTFILE(),
Copy link
Owner

Choose a reason for hiding this comment

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

There should be a better way to clear it. Why StandardOpenOption.CREATE? Why not simply delete it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Deleting it and then trying to recreating it causes security exception on Windows

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 9a4e5f5

@@ -114,7 +114,11 @@ object Player: FadingHBox(true, targetHeight = 25) {
fun reset() {
fadeOut()
if(!Files.isDirectory(Settings.PLAYEREXPORTFILE())) {
Copy link
Owner

Choose a reason for hiding this comment

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

What if it is a directory? Doesn't look like that's handled

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why should it need to? If it's a directory, we could show an error message but that would be horrible for the user as it'll show each time a write is made

Copy link
Owner

@xeruf xeruf Nov 13, 2020

Choose a reason for hiding this comment

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

But silent failure seems like unexpected behavior as well. How about adding a debug log message?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How about adding an extension to the file in case it's already a folder?

src/main/xerus/monstercat/api/Player.kt Outdated Show resolved Hide resolved
@defvs defvs marked this pull request as draft November 11, 2020 13:59
@defvs
Copy link
Collaborator Author

defvs commented Nov 11, 2020

Changing to draft; going to implement some new feature as well.

@defvs defvs changed the title Fix Export Current Track to File issues on Windows Fix Export Current Track and add format option Nov 11, 2020
@defvs defvs changed the title Fix Export Current Track and add format option player: Fix Export Current Track and add format option Nov 11, 2020
@defvs defvs marked this pull request as ready for review November 11, 2020 14:45
@defvs defvs requested a review from xeruf November 11, 2020 14:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants