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

Fixes + Tweaks #396

Merged
merged 15 commits into from
Nov 8, 2023
Merged

Fixes + Tweaks #396

merged 15 commits into from
Nov 8, 2023

Conversation

nlogozzo
Copy link
Member

@nlogozzo nlogozzo commented Nov 6, 2023

Fixes #395

TODO:

  • Fix Tagger ignoring space with invalid chars

WinUI Tweaks:

  • Removed Apply button for SyncLyricRow. Lyric will be automatically updated as the text box changes
  • Make add sync lyric a flyout instead of showing new ContentDialog
  • See where else flyouts can be used instead of full ContentDialogs

@nlogozzo nlogozzo changed the title Fixes Fixes + Tweaks Nov 6, 2023
@nlogozzo nlogozzo marked this pull request as ready for review November 6, 2023 21:45
@nlogozzo
Copy link
Member Author

nlogozzo commented Nov 6, 2023

@nlogozzo nlogozzo marked this pull request as draft November 6, 2023 22:13
@kissthermite
Copy link

@kissthermite Could you test this build: https://github.com/NickvisionApps/Tagger/actions/runs/6776895782

sure! will let you know in a moments

@kissthermite
Copy link

Using custom format string /%albumartist%/%album%/%artist% - %title% with Limit File Name Characters setting turn on does work as expected for others limited characters except /


tag to file name conversion with aforementioned format string with limited character being :

will give this output: /Kotono Nagase (CV_Mirai Tachibana)/song for you(Kotono version)/Kotono Nagase (CV_Mirai Tachibana) - song for you(Kotono version).flac


tag to file name conversion with aforementioned format string with limited character being /

will give this output: /SQUARE ENIX MUSIC/NieR Gestalt & NieR Replicant Original Soundtrack/SQUARE ENIX MUSIC - Song of the Ancients /Devola.flac


@nlogozzo
Copy link
Member Author

nlogozzo commented Nov 7, 2023

tag to file name conversion with aforementioned format string with limited character being /

Well now with the new algorithm / means new directory. But you want it where if the / was contained in a tag property and now the format string, to not count as a new directory, correct?

I think I can do that.

@kissthermite
Copy link

But you want it where if the / was contained in a tag property and now the format string, to not count as a new directory, correct?

Correct indeed.

@nlogozzo
Copy link
Member Author

nlogozzo commented Nov 7, 2023

@fsobolev If you could make sure the algorithm in this commit still holds: 325b0f4

@nlogozzo
Copy link
Member Author

nlogozzo commented Nov 7, 2023

But you want it where if the / was contained in a tag property and now the format string, to not count as a new directory, correct?

Correct indeed.

@kissthermite Try this build: https://github.com/NickvisionApps/Tagger/actions/runs/6779510126?pr=396

P.S. Thanks for testing and sorry for messing up your library structure with these tests 🫠

@nlogozzo nlogozzo marked this pull request as ready for review November 7, 2023 03:16
@nlogozzo nlogozzo requested review from fsobolev and removed request for fsobolev November 7, 2023 03:32
@nlogozzo
Copy link
Member Author

nlogozzo commented Nov 7, 2023

But you want it where if the / was contained in a tag property and now the format string, to not count as a new directory, correct?

Correct indeed.

@kissthermite Try this build: https://github.com/NickvisionApps/Tagger/actions/runs/6779510126?pr=396

P.S. Thanks for testing and sorry for messing up your library structure with these tests 🫠

Sorry I lied, please try this build: https://github.com/NickvisionApps/Tagger/actions/runs/6779656223

If a tag property contains /, regardless of using Limit Filename Characters, it will not create a new directory. Only if the format string explicitly contains a /, then it will.

@kissthermite
Copy link

kissthermite commented Nov 7, 2023

P.S. Thanks for testing and sorry for messing up your library structure with these tests 🫠

Don't worry my actual music library are stored on /var/home/kissthermite/Jellyfin Server Media/Music

The one on Downloads folder are just a copy for testing purpose XD.

@kissthermite
Copy link

kissthermite commented Nov 7, 2023

If a tag property contains /, regardless of using Limit Filename Characters, it will not create a new directory. Only if the format string explicitly contains a /, then it will.

Just tested and indeed it does behave the way you said whether Limit File Name Characters turn on/off. Good job @nlogozzo! and enjoy your well deserved rest tonight.

@nlogozzo
Copy link
Member Author

nlogozzo commented Nov 7, 2023

Just tested and indeed it does behave the way you said whether Limit File Name Characters turn on/off. Good job @nlogozzo! and enjoy your well deserved rest tonight.

Yay, glad it's working as expected now! Once @fsobolev approves of the rest of this PR, I will release a beta.

If you have any other issues/feature requests please try to report them before Wednesday that way I can implement them all on Wednesday and then get a stable out by Friday 😅

@kissthermite
Copy link

If you have any other issues/feature requests please try to report them before Wednesday that way I can implement them all on Wednesday and then get a stable out by Friday 😅

Actually there's one thing.

With Limit File Name Characters turn on, Tagger ignored space before, between & after that limited characters, is that the intended behavior?

@nlogozzo
Copy link
Member Author

nlogozzo commented Nov 7, 2023

With Limit File Name Characters turn on, Tagger ignored space before, between & after that limited characters, is that the intended behavior?

Hmm...no it's not intended. Will take a look at it tomorrow to see what's going on.

NickvisionTagger.Shared/Models/MusicFile.cs Outdated Show resolved Hide resolved
NickvisionTagger.Shared/Models/MusicFile.cs Outdated Show resolved Hide resolved
@nlogozzo
Copy link
Member Author

nlogozzo commented Nov 8, 2023

With Limit File Name Characters turn on, Tagger ignored space before, between & after that limited characters, is that the intended behavior?

@kissthermite Can't reproduce...
Here's WinUI:
image
Here's GNOME:
image

I think it just seems every small in GNOME but the space is indeed there.

@kissthermite
Copy link

Yeah. My bad just check it again and indeed it's just a very tiny space.

@nlogozzo nlogozzo merged commit 8e0c6e1 into main Nov 8, 2023
@nlogozzo nlogozzo deleted the next branch November 8, 2023 02:23
@nlogozzo
Copy link
Member Author

nlogozzo commented Nov 8, 2023

I was going to release a beta but decided against it since it's almost Friday and if any bugs come up before then I will fix them and just release stable. Since your issue was confirmed to be fixed, no need for a beta.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants