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

Share Lizzy playlist implementation #144

Merged
merged 8 commits into from
Mar 31, 2023

Conversation

Borewit
Copy link
Owner

@Borewit Borewit commented Mar 19, 2023

Changes:

Motivates:

  • Better modular architecture
    • Removes duplicate code, and therefor maintenance, between Lizzy and listFix()
    • Highly increases changes the playlist Lizzy code will reused be other projects, which will increase the opensource community involvement, which will increase quality
    • Reduced the Java code size of listFix() from 17970 to 14474 lines, which reduced the Java code size with 3496 lines (tests not included)
    • I modernized Lizzy, replacing Castor XML to JAXB 3:
      • Eliminated a security vulnerability dependency Castor XML, which seems no longer being updates
      • Reduced the Java code size from Lizzy 33642 to 10853, which are 22799 lines code less
  • Access to formats Lizzy supports
  • I believe the serialization and de-serialization also became faster

Resolves:

ToDo:

  • Include Lizzy playlist types
  • Release 'io.github.borewit:lizzy:2.0.0', currently on SNAPSHOT version

@Borewit Borewit added debt Technical debt dependencies Pull requests which update a dependency file labels Mar 19, 2023
@Borewit Borewit self-assigned this Mar 19, 2023
@sonatype-lift
Copy link

sonatype-lift bot commented Mar 19, 2023

🛠 Lift Auto-fix

Some of the Lift findings in this PR can be automatically fixed. You can download and apply these changes in your local project directory of your branch to review the suggestions before committing.1

# Download the patch
curl https://lift.sonatype.com/api/patch/github.com/Borewit/listFix/144.diff -o lift-autofixes.diff

# Apply the patch with git
git apply lift-autofixes.diff

# Review the changes
git diff

Want it all in a single command? Open a terminal in your project's directory and copy and paste the following command:

curl https://lift.sonatype.com/api/patch/github.com/Borewit/listFix/144.diff | git apply

Once you're satisfied, commit and push your changes in your project.

Footnotes

  1. You can preview the patch by opening the patch URL in the browser.

@Borewit
Copy link
Owner Author

Borewit commented Mar 19, 2023

@touwys, sorry for working a bit out of sync. I hope the description of this PR gives you a bit of understanding why I need to make this drastic changes.

Test version relfecting these changes: listFix_2.6.0-PR144-35.exe.

Please backup your valuable playlist files before touching them with this version.

Special points of attention (things which may have broken):

  • Copy pasting, dragging
  • Saving, loading, converting

@touwys
Copy link

touwys commented Mar 19, 2023

I hope the description of this PR gives you a bit of understanding why I need to make this drastic changes.

No problem. I did notice that you removed a lot of code for listFix(), and that's about it. I can see you removed a lot of code for listFix(), and that's about it. As for the rest, I am just guessing what is going on. I do not have a proper understanding of just how Lizzy fits into your plans — as far as liftFix() is concerned. I can only assume that Lizzy will be somehow used for converting playlist formats upon exporting them.


Please backup your valuable playlist files before touching them with this version.

Special points of attention (things which may have broken):

* Copy pasting, dragging

* Saving, loading, converting

Thanks for the heads-up.


ℹ️ Borewit: The downloading of the files you upload to MediaFire sometimes takes much, much too long — as previously noted. I think they throttle unregistered clients, as and when required. The projected download time for the current file (74.34 MB), is a crazy, approx. 2 hours — and that is not taking unexpected power interruptions into account, of which we've had several today, already. Since this is the case, I again strongly suggest that we test another service provider, such as Workupload., and then let's see if we can gain on the download speed. Please, upload this same file there, so I can test the viability of their service?


image

@touwys
Copy link

touwys commented Mar 19, 2023

Testing: listFix_2.6.0-PR144-35

  1. False 'Root' directory at start-up. See screenshot A. This shows up when the app is started for the very first time — before any playlist directories, or media directories, are added. It cannot be deleted in the usual manner.

  2. Drag and drop folders into PLD is impossible, but it can be added via "Add" button using Windows explorer to locate the playlist folders.

  3. Open playlist issue: See screenshot B. Error when opening playlist into the Playlist Editor

I stopped testing at his point to wait for further instructions?


rollingfile.log


Screenshot A

PLD 'Root' _ Screenshot 20230319_2054_59


Screenshot B

Open playlist error _ Screenshot 20230319_2109_17

@Borewit Borewit force-pushed the share-lizzy-playlists-implementation branch 3 times, most recently from 2f0bfde to b397971 Compare March 20, 2023 15:24
@Borewit
Copy link
Owner Author

Borewit commented Mar 20, 2023

is a crazy, approx. 2 hours

What?! 8 kB/s?...

Although playlists belong to the 90's, let's not get stuck with the the download speed of those days!
I am sorry for torturing you with MediaFire, I won't use it anymore. The South African power-grid is beyond my reach.

The current distribution is about 80 MB, it should technically be possible to bring that back to ~25 MB, if we find a way to wrap the compiled distribution in an installer.

Updated distribution (despite the same name, not the same as previous release!) listFix_2.6.0-35.exe

@touwys
Copy link

touwys commented Mar 20, 2023

OK, thanks. This time the download took only a few minutes. Let's hope it stays that way. Only time will tell, because there are many outside factors that also infuence the quality of local operations.

I aim to test the latest build, tomorrow.

@Borewit Borewit force-pushed the share-lizzy-playlists-implementation branch from b397971 to d178d44 Compare March 20, 2023 21:05
@Borewit
Copy link
Owner Author

Borewit commented Mar 20, 2023

I did some more work.
Now included all the support playlist types Lizzy supports.

I improved the save-as dialog, automatically adjusting the file extension based the file-type choosed.

Loading works well, looks like some file formats do no save properly and resulting in empty playlists.

listFix_2.6.0-35.exe

@touwys
Copy link

touwys commented Mar 21, 2023

I did some more work.
Now included all the support playlist types Lizzy supports.

@Borewit:

👍🏻 That you have added support for Lizzy, is truly beneficial. I realise the very usefulness of its deployment. However, how do you propose I should go ahead and test Lizzy's features, because I am really only using one small fraction of its formats, that is, m3u?

🚀 It was a nice, faster file download from workupload.com. Thanks.

@touwys
Copy link

touwys commented Mar 21, 2023

Testing: listFix_2.6.0-35

1. Playlist Editor:

1.1 Confirm Reload dialogue > Discard changes and reload of a playlist is broken. The command is not executed.

1.2 The playlist layout (the sequence, plus the one last active) is not restored at the restart.

2. Select Closest Matches Dialogue:

2.1 Preview / Play is broken. The command is not executed.

2.2 Open match file location is broken. The command is not executed.

3. Editing a Playlist > Context Menu:

3.1 Edit Filename dialogue: Check for unexpected, erratic behaviour when pressing the "Cancel" button?

3.2 Create New Playlist with Selected Items: Opens a new playlist tab with "m3u8" file format. Why does this format not mirror the format of the playlist it was cloned from? In addition, why is it not otherwise possible to save (or convert) the newly created playlist, other than upon closing it? How about selecting and then saving from the context menu? Furthermore, this is one place where a tab context menu, with some basic functions incorporated, would come in very handy.

I did not proceed with the test, beyond the aforesaid.


ℹ️ Open file location (from a file picked from the playlist that is currently open in the Editor): The located file is not highlighted at its origin. Can you please try to incorporate early support for this important function?


👉🏻 Adding the following result:

After saving a newly created playlist to another format, and adding it back to the PLE by dragging and dropping it, it fails — view the screenshot below for the error message:


rollingfile.log

image

@touwys
Copy link

touwys commented Mar 21, 2023

@Borewit :

Please note that the playlist tab titles still disappear when changing Options > Theme

@Borewit
Copy link
Owner Author

Borewit commented Mar 21, 2023

how do you propose I should go ahead and test Lizzy's features, because I am really only using one small fraction of its formats, that is, m3u?

We are probably decades beyond the real added value of supporting those ancient playlist formats. The most exotic ones already I kicked out. Yet I felt the need to preserve the other, also because there are no existing Java modules for this.

The good thing is that I changed listFix so that it is now almost irrelevant what the underlying playlist format.

Maybe one day I add feature which allows to import Spotify playlists, which essentially would allow to convert the online playlist to an offline playlist.

I am really only using one small fraction of its formats, that is, m3u

No worries. I can cover a lot of that with unit tests.

@Borewit Borewit force-pushed the share-lizzy-playlists-implementation branch from d178d44 to 4cf8eea Compare March 23, 2023 19:48
@Borewit
Copy link
Owner Author

Borewit commented Mar 23, 2023

Please note that the playlist tab titles still disappear when changing Options > Theme

I will try to resolve that in another PR.

This version should address issues introduced with this PR: listFix_2.6.0-35.exe

@Borewit Borewit force-pushed the share-lizzy-playlists-implementation branch 2 times, most recently from d3d75e1 to 3a188f2 Compare March 24, 2023 09:52
@touwys
Copy link

touwys commented Mar 24, 2023

I will try to resolve that in another PR.

I think that you should not worry about this too much, since we discussed previously that the "Themes", as a GUI, do not have much bearing on the more essential operations. I only mentioned it in passing as a reminder that is still broken, thinking that you may want to investigate to see if the interference is related to the current PR-development issues. Normally, I very seldom fiddle with the Themes.


This version should address issues introduced with this PR: listFix_2.6.0-35.exe

This file is not available to download. Did you withdraw it?


@touwys
Copy link

touwys commented Mar 26, 2023

@Borewit:

This file is not available to download. Did you withdraw it?

Off-topic: I noticed that you also wrote this Musicbrainz-API. Is it perhaps the one that is also utilised by Mp3tag?

@Borewit
Copy link
Owner Author

Borewit commented Mar 26, 2023

This version should address issues introduced with this PR: listFix_2.6.0-35.exe

❓ This file is not available to download. Did you withdraw it?

That link is working just fine. In addition to that, here is the very latest listFix_2.6.0-35.exe. Minor change, I formally released Lizzy 2.0.0 and depend now on that released version. As I keep overwriting the commit of this PR, the commit distance, 35, (version) remains the same.

Off-topic: I noticed that you also wrote this Musicbrainz-API. Is it perhaps the one that is also utilised by Mp3tag?

I don't think so, Mp3tag existed long before I wrote Musicbrainz-API. I also wrote music-metadata most popular JavaScript music metadata parser (used in almost 25000 open source projects, but also shipped in high end audio equipment) and I am the main contributor file-type (1.7 million open source project have been using that). To bad I can not utilize them in listFix as they are written in a different language.

@touwys
Copy link

touwys commented Mar 26, 2023

That link is working just fine. In addition to that, here is the very latest listFix_2.6.0-35.exe. Minor change, I formally released Lizzy 2.0.0 and depend now on that released version. As I keep overwriting the commit of this PR, the commit distance, 35, (version) remains the same.

You're right, the link works just fine. I tracked the problem down to a new ad-blocker extension I installed for the browser. I am downloading the latest build now. Download speed is down again, just like it was with MediaFire. Nothing to be done, so it seems, so I will stick at it out for a time to see what gives.


I don't think so, Mp3tag existed long before I wrote Musicbrainz-API. I also wrote music-metadata most popular JavaScript music metadata parser (used in almost 25000 open source projects, but also shipped in high end audio equipment) and I am the main contributor file-type (1.7 million open source project have been using that). To bad I can not utilize them in listFix as they are written in a different language.

👍🏻 I saw some of those on your profile. You are indeed a very prolific developer. I am also very glad that you decided to take another look at oldie listFix() — 😀. I do hope that you get to smooth out all its wrinkles early on, and I am happy to assist you where I can. Are you a full-time programmer/developer, employed in a corporate environment?


@touwys
Copy link

touwys commented Mar 27, 2023

Testing listFix_2.6.0-35

  1. Drag to drop a playlist from its PLD-folder into the PLE, is broken.

  2. Select Closest Matches dialogue > Preview > PLAY is broken. (Sidenote: The play button in the PLE toolbar, however, does call the default music player to complete such a request.).

  3. Select Closest Matches dialogue > Open match file location is broken.

  4. Reload > Discard Changes and Reload — The playlist does not discard the changes made, nor reload.

  5. Restoring the playlist-tabs layout — The correct sequence of the tabs is restored, but the one last active is not getting restored again as active. Instead, the tab, last in the sequence, gets restored as the one active.


Observations | Reminders:

  1. File > Options > Look and Feel — App does not start with the default theme. Uses "Windows (Plastic)" instead.

  2. File > Options > Font — Some app graphic elements do not adapt to changes in the font style/size. For e.g. Select Closest Matches dialogue > Preview > PLAY button.

  3. Drag and drop individual playlist into PLD is not yet possible?


Testing did not go beyond the aforesaid; waiting for the fix.


rollingfile.log

Remove all internal marshalling and unmarshalling of playlists, use Lizzy module instead.
Update to dependency `io.github.borewit:lizzy` to version 2.0.0
@Borewit Borewit force-pushed the share-lizzy-playlists-implementation branch from 362b85a to c69775a Compare March 27, 2023 10:50
@Borewit
Copy link
Owner Author

Borewit commented Mar 27, 2023

Fixed:

  • Drag to drop a playlist from its PLD-folder into the PLE, is broken.
  • Select Closest Matches dialogue > Preview > PLAY is broken. (Sidenote: The play button in the PLE toolbar, however, does call the default music player to complete such a request.).
  • Select Closest Matches dialogue > Open match file location is broken.
  • Reload > Discard Changes and Reload — The playlist does not discard the changes made, nor reload.
  • Restoring the playlist-tabs layout, out-of-scope in this PR, issue captured in Persist and restore last active tab  #142

I fixed a bug which would result in an invalid play-playlist, which could explain the issues you had playing back.
Not sure if I fixed the closest matches dialogue, or if fail to reproduce it. Seems to be okay now.

Drag and drop individual playlist into PLD is not yet possible?

You can only drop directories in that component, it does not make sense to me to accept playlists. What should the playlist directory panel do with a playlist? You can drag playlists into the editor. Anyway, out of scope of this PR.

Would you mind to perform another regression test? As long this PR does not introduce new issues, it is good to go.

listFix_2.6.0-41.exe on WeTransfer
listFix_2.6.0-41.exe on workupload.com

@Borewit
Copy link
Owner Author

Borewit commented Mar 29, 2023

employed in a corporate environment?

Indeed, but the days I am really doing some hands-on coding are gone. Nowadays I mostly do management work, software architecture and leading a small development team.

@Borewit
Copy link
Owner Author

Borewit commented Mar 29, 2023

@touwys do you have time to review this? Otherwise I proceed with merging.

@touwys
Copy link

touwys commented Mar 30, 2023

@touwys do you have time to review this? Otherwise I proceed with merging.

@Borewit

Oops, sorry! 😏. Somehow I failed to notice that download. I will try get to it, and let you know asap. Will download the file from both those sources in order to compare their performance from here. Thanks.

@touwys
Copy link

touwys commented Mar 30, 2023

listFix_2.6.0-41

image

✔️ Both items now fixed.


rollingfile.log


Comments & Observations

  1. Stick to workupload.com for listFix() build uploads — at least for the time being.

  2. It's not that important, but for consistency’s sake, why not add drag and drop Media folders onto the Media Directories panel, as well? Want me to open an issue for this? It's low priority — if at all.

  3. File > Options > Number of Closest Matches to Return when Searching: Realistically speaking, I think the default number should be trimmed down to 5 or 10 max. I do not know why it was set so high, at 20?

You can only drop directories in that component, it does not make sense to me to accept playlists.

  1. Drag and drop of playlists — not folders — onto the PLD was just my natural reaction while I was at it. I agree, it does not make sense — unless, after the playlist is closed in the Editor, we still want to keep it nearby on hand in the playlist directory?

@Borewit
Copy link
Owner Author

Borewit commented Mar 30, 2023

Thanks for your review @touwys

Stick to workupload.com for listFix() build uploads — at least for the time being.

Okay

why not add drag and drop Media folders onto the Media Directories panel, as well? Want me to open an issue for this? It's low priority — if at all.

But that has been implemented in PR #117 PR #113, maybe I don't understand what you want.

File > Options > Number of Closest Matches to Return when Searching: Realistically speaking, I think the default number should be trimmed down to 5 or 10 max. I do not know why it was set so high, at 20?

I agree. Micro change, but I do it in a different PR, so we have the change documented.

Drag and drop of playlists — not folders — onto the PLD was just my natural reaction while I was at it. I agree, it does not make sense — unless, after the playlist is closed in the Editor, we still want to keep it nearby on hand in the playlist directory?

If you close all playlists, you can still drag playlists into the editor. The only difference is, that you are not asked to insert the playlists into the active playlists. I think I have done that as part of PR #136.

Not sure to which items you refer with "both items now fixed", are these 2 also addressed:

  • Select Closest Matches dialogue > Preview > PLAY is broken. (Sidenote: The play button in the PLE toolbar, however, does call the default music player to complete such a request.).
  • Select Closest Matches dialogue > Open match file location is broken.

If not, can you help me to reproduce the problems?

@Borewit
Copy link
Owner Author

Borewit commented Mar 30, 2023

And I noticed an error in your log file, this one is most likely introduced in this PR:

2023-03-27 08:39:56.020 ERROR [AWT-EventQueue-0] listfix.view.GUIScreen - Uncaught Exception
java.lang.ClassCastException: class sun.nio.fs.WindowsPath cannot be cast to class listfix.model.playlists.FilePlaylistEntry (sun.nio.fs.WindowsPath is in module java.base of loader 'bootstrap'; listfix.model.playlists.FilePlaylistEntry is in unnamed module of loader 'app')
	at listfix.view.controls.ClosestMatchesSearchScrollableResultsPanel.openClickedMatchedPlayListEntryLocation(ClosestMatchesSearchScrollableResultsPanel.java:462) ~[listFix.exe:2.6.0-35]
	at listfix.view.controls.ClosestMatchesSearchScrollableResultsPanel.lambda$initComponents$0(ClosestMatchesSearchScrollableResultsPanel.java:449) ~[listFix.exe:2.6.0-35]
	at javax.swing.AbstractButton.fireActionPerformed(Unknown Source) ~[?:?]
	at javax.swing.AbstractButton$Handler.actionPerformed(Unknown Source) ~[?:?]
	at javax.swing.DefaultButtonModel.fireActionPerformed(Unknown Source) ~[?:?]

@touwys
Copy link

touwys commented Mar 30, 2023

But that has been implemented in PR #117 PR #113, maybe I don't understand what you want.

I am referring to Media folders getting dragged into the Media Directories Panel. #117 refers to the Playlist Directory Panel?

If you close all playlists, you can still drag playlists into the editor.

Not so, if the closed playlists were originally dragged from windows explorer, and not the Playlist Directory Panel.

Not sure to which items you refer with "both items now fixed"

My statement follows directly under the screenshot above it, in which the 1st and 4th items that are checkmarked. These two are now fixed.

are these 2 also addressed:

Select Closest Matches dialogue > Preview > PLAY is broken. (Sidenote: The play button in the PLE toolbar, however, does call the default music player to complete such a request.).
Select Closest Matches dialogue > Open match file location is broken.

If not, can you help me to reproduce the problems?

I will surely do. Both the aforesaid are broken. What do you need from me to be able to reproduce the error? Do screen videos help, or is there something else you need me to do?

@touwys
Copy link

touwys commented Mar 30, 2023

And I noticed an error in your log file, this one is most likely introduced in this PR:

I am not at the PC again, until tomorrow. Does it refer to the action Open File Location issue?

@Borewit
Copy link
Owner Author

Borewit commented Mar 30, 2023

Fixed 2 issues: listFix_2.6.0-43.exe

I made small change in playing matched entries, I directly open the audio track instead of wrapping it in a temporary playlist (m3u) and opening that one. I think that is a bit old school. So if you see different behavior opening the track from there, that is why.

@touwys
Copy link

touwys commented Mar 31, 2023

Review: listFix_2.6.0-43

✔️ Summary: All the issues listed in the screenshot below, are now fixed.

image


I made small change in playing matched entries, I directly open the audio track instead of wrapping it in a temporary playlist (m3u) and opening that one. I think that is a bit old school. So if you see different behavior opening the track from there, that is why.

Good work, this made a considerable difference: If I am not mistaken, the default player is called notably faster. Previously, this process was quite sluggish.


rollingfile.log

What's up next?

@Borewit Borewit merged commit 9861d60 into main Mar 31, 2023
@Borewit Borewit linked an issue Mar 31, 2023 that may be closed by this pull request
@Borewit
Copy link
Owner Author

Borewit commented Mar 31, 2023

I am referring to Media folders getting dragged into the Media Directories Panel.

Makes total sense to add that. I was convinced I already implemented that, but I checked both the default branch and this branch, and as you said, this is not possible yet.

What's up next?

I propose to:

  1. Persist and restore last active tab  #142
  2. Change the default of closest match to 5
  3. Add the support for dragging media folders

In principal only address bugs and very low hanging fruit, and then release.

@Borewit Borewit deleted the share-lizzy-playlists-implementation branch March 31, 2023 10:58
@touwys
Copy link

touwys commented Mar 31, 2023

I propose to:
2. Change the default of closest match to 5

@Borewit

Looking good! On item 2 above: is this particular change at all related to the efficiency of the maching tracks search engine? Because, if it is, will it not be worthwhile to wait until you have improved the search accuracy to get better matches? Refer to: #120


Miscellaneous:

  1. I spent much time reading how to highlight text in Github messages, but to no avail. I did notice that you are doing it. Do you mind sharing the method?

  2. Sometimes, it is probably better to go off-board to discuss something. If you agree to it, and wish to correspond via email, please find my address over here.

Happy weekend!

@Borewit
Copy link
Owner Author

Borewit commented Mar 31, 2023

highlight text in Github messages

I am not aware of any highlight function, maybe you mean:
You mean `code / mono-space` for code / mono-space
or **bold** for bold
or _italic_ for italic

@touwys
Copy link

touwys commented Mar 31, 2023

My bad, I meant colour highlighting of any text, for example, in bbcode, like this:

[Color=blue]Hightlight this text[/color]

Github seems to not allow text colour formatting, bar under very special circumstances.

@Borewit
Copy link
Owner Author

Borewit commented Mar 31, 2023

My bad, I meant colour highlighting of any text, for example, in bbcode, like this:

[Color=blue]Hightlight this text[/color]

Github seems to not allow text colour formatting, bar under very special circumstances.

I see what you mean, but I don't think I am using that or that it available. Are you sure you had no open search query in your browser, which highlighting the text searched?

@touwys
Copy link

touwys commented Apr 1, 2023

I see what you mean, but I don't think I am using that or that it available. Are you sure you had no open search query in your browser, which highlighting the text searched?

An example of the text highlighting is visible at the one of the preceding messages in this very string: Screenshot

I conclude that the text colour highlighting is probably auto-generated by Git. Thanks for your input.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debt Technical debt dependencies Pull requests which update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Playing a file from a playlists throws an error in VLC
2 participants