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

Refactor playlist writers with logging #47

Merged
merged 20 commits into from
Feb 5, 2023

Conversation

Borewit
Copy link
Owner

@Borewit Borewit commented Feb 1, 2023

Massive refactoring of code:

  • Playlist writers
    • Prevent code duplication, use common abstract structure for heading, entries, and finalization
  • No longer pass IPlaylistOptions to playlist-entries
  • Respect relative path doing exact match repairing

Fixes #45, #58

@Borewit Borewit self-assigned this Feb 1, 2023
@Borewit
Copy link
Owner Author

Borewit commented Feb 1, 2023

@touwys, I got a new release for testing: listFix_2.5.1-3.exe

@touwys
Copy link

touwys commented Feb 2, 2023

listFix_2.5.1-3

  • This release was installed over the previous one, listFix_2.5.1-1

  • The same set of playlists, previously used, remained for testing in the Playlist Directory

  • The test started by trying to open the Context Menu of the 3rd m3u- of the playlist down from the top of the directory, into the Editor Pane (Repair Window) on the right.

The test failed outright, as an error message was instantly flagged upon opening the playlist for repair.

The graphical error text message, converted to text, plus the app log-files, and plus a composite screenshot containing the relevant information, are presented below:

  1. Problem opening the file you selected, are you sure it was a playlist.txt
  2. rollingfile.log
  3. logfile.log

SCREENSHOT

@Borewit Borewit force-pushed the refactor-playlist-writers-with-logging branch from 7bc0542 to c8f492d Compare February 2, 2023 18:32
@Borewit
Copy link
Owner Author

Borewit commented Feb 2, 2023

At least that was showing a proper error when an error occured.
My mistake, introduced that one in my last changes.

Should be fixed in this release: listFix_2.5.1-9.exe

I did fix issue #45, now respecting relative path, and no longer keep converting back and forward. I did notice that opening and closing playlists triggers a message, this playlist has been changes, while it didn't, need to fix that.

The folder paths should be truncated, they should show the way they are in the playlist.

I only kept the rollingfile.log as this is more then enough for now. You may delete the other one.
Reinstalling seems to work fine, so don't bother uninstalling things in the future.

@touwys
Copy link

touwys commented Feb 2, 2023

The test started by trying to open the Context Menu of the 3rd m3u- of the playlist down from the top of the directory, into the Editor Pane (Repair Window) on the right.

I should have added to the above that after trying to open the 3rd playlist, which failed, I opened the 2nd one from the top, and, lastly, the 1st one from the top. The 1st one also failed like the 3rd, but the 2nd one did open — only I did not proceed to repair it, thinking the truncation was an error, too. All the results are reflected in the Screenhot posted above.

It should be noted that the 2nd playlist that says "edit" in its description, is the one that was made from a compilation of music files, which were exported to another folder, outside the music library, which is managed by MusicBee.

I suppose the 2nd playlist did not relative paths like the other two did? Is that what broke the other two?

@touwys
Copy link

touwys commented Feb 3, 2023

Testing listFix_2.5.1-9

Two m3u-playlists were tested — only as far as the results below show:

Test Playlist 1: ("OH")

A. Applied "Find Exact Matches":

A1. — It found all, but two tracks.

B. Applied "Fix Everything">"Select Closest Matches":

B1. — It found two very close matches for the two broken tracks. However, activating the "Preview">"Play" buttons failed to call up the default music player for track quality assessment.

B2. — The music player was consequently called up manually, and operation 2.1 was repeated. "Preview">"Play" tracks still did not function.

Note on B1, B2: This behaviour was also noticed with some of the previous releases, but it was fixed. Probably a regression?

Test Playlist 2: ("Compilation")

C. Applied "Find Exact Matches":

C1. — It found none. It started off with the "Repairing" progress window getting displayed, and the progress bar finishing before it closes.

Note on C1: Should the user just assume that the "Find Exact Matches" operation completed properly like it should, in the absence of any form of confirmation to the fact that it did, or did not, complete the operation successfully?

D. The "Repairing" progress window ran first, followed by "Finding Closets Matches for all missing files..." It found the closest matches for all the tracks, quickly.

Note on D [edit]: The locations of the tracks in this playlist, were NOT getting displayed in the right-hand pane. Is this a bug, or is there another explanation for it?


rollingfile.log

@Borewit Borewit force-pushed the refactor-playlist-writers-with-logging branch 4 times, most recently from 984a8f0 to aba676d Compare February 3, 2023 13:52
@Borewit
Copy link
Owner Author

Borewit commented Feb 3, 2023

listFix_2.5.1-16
Playback issues should be solved (B1, B2).
Also did some refactoring in the playlist repair, not sure it fixed C1 & D1, if you can please test those one for time.

@touwys
Copy link

touwys commented Feb 4, 2023

Playback issues should be solved (B1, B2).

listFix_2.5.1-16:

The same three m3u-playlists that were used in the previous tests, were used utilised for this test, as well. I only tested as far as B1 & B2 is concerned.

The results were not as expected. Once the Closest Matches were found, the Preview>Play would sometimes start up the closed default music player to play the selected track — and sometimes it did not. Also, if the default player is open already, many a selected track Preview>Play would not show up in the playlist to start playing — i.e. there was no player reaction to the input when clicking the track Preview>Play button.


rollingfile.log

(I have yet to test listFix_2.5.1-[PR51]-4.exe)

@Borewit
Copy link
Owner Author

Borewit commented Feb 4, 2023

Note on D1: The locations of the tracks in this playlist, were NOT getting displayed in the right-hand pane. Is this a bug, or is there another explanation for it?

That depends on your application setting and playlist content.
The location is the folder prefixing the track filename, as appears in your playlist file.

So if your playlist files has a path Madonna/Little Prayer.mp3, it should be displayed as:
Little Prayer.mp3, Madonna.

Madonna Little - Prayer.mp3, should be displayed as:
Madonna Little - Prayer.mp3, empty.

C:\music\Madonna\Little Prayer.mp3, should be displayed as:
Little Prayer.mp3, C:\music\Madonna.

..\Madonna\Little Prayer.mp3, should be displayed as:
Little Prayer.mp3, ..\.

@touwys
Copy link

touwys commented Feb 4, 2023

Thank you for explaining that. I never before considered opening a playlist in a text viewer.

@Borewit
Copy link
Owner Author

Borewit commented Feb 4, 2023

As B1 & B2 are no issues introduced by this PR (see also this comment on PR#51).

Thank you for explaining that. I never before considered opening a playlist in a text viewer.

Oh this is very useful, but better do that with notepad++ and not with your default notepad.

When you play a single, it creates a .m3u playlist in a temporary folder, and then calls the OS to "open" that playlist with file you want to play. So that is something we need to look into, in the other issue.

D. The "Repairing" progress window ran first, followed by "Finding Closets Matches for all missing files..." It found the closest matches for all the tracks, quickly.

I was contused about that one, I know what you mean, but I was worried I changed something linked the processed with each other. Went back to 2.4.0, and it the same. There in one difference, 2.4.0 is much faster. It could be that the logging slows things a bit down. Speed is not the most important at the moment.

I C1. — It found none. It started off with the "Repairing" progress window getting displayed, and the progress bar finishing before it closes.

I broke something in the progress bar, causing it to go back and forward, fixed that in 9f9b709.

@Borewit
Copy link
Owner Author

Borewit commented Feb 4, 2023

Latest build listFix_2.5.1-[PR47]-21.exe

@touwys
Copy link

touwys commented Feb 4, 2023

Just to be certain: Is listFix_2.5.1-[PR47]-21.exe a newer build than listFix_2.5.1-[PR51]-7.exe ? Does mean it is a later/higher branch takes precedence over the higher PR ? It's all quite confusing to a novice. 🥴

@touwys
Copy link

touwys commented Feb 4, 2023

Oh this is very useful, but better do that with notepad++ and not with your default notepad.

Notepad++ it is, just as you previously recommended. Thanks!

The default Windows notepad is very rarely used. I replaced it with NoteTab and later on, for convenience, the text editor built into xplorer² — a Windows File Manager (Explorer replacement). It works a treat, too.

@Borewit
Copy link
Owner Author

Borewit commented Feb 4, 2023

Different branches, to easier avoid mixing up the branches I have added the PR identifiers "[47]" and "[51]".

Branches drawio

When you believe the [47] is okay, I merge it to the default branch. You can compare the branch with track changes in Word.

@Borewit
Copy link
Owner Author

Borewit commented Feb 5, 2023

@touwys , as PR #51 has now been merged to the default branch, I am now going to rebase this branch / PR on top of the default branch....

@Borewit Borewit force-pushed the refactor-playlist-writers-with-logging branch from bb59c88 to 3de7548 Compare February 5, 2023 12:38
@Borewit
Copy link
Owner Author

Borewit commented Feb 5, 2023

Branch has now been rebased:

Tree after merging drawio

Giving you a new release: listFix_2.5.1-[PR47]-25.exe

Any problems remaining from PR #51, (like tabs) should be created as new issue. If you have no strong objections, I also would like to merge this branch.

@touwys
Copy link

touwys commented Feb 5, 2023

@Borewit:

Thank you for the new release. I will likely only be in a position to attend the new build tomorrow. Sooner when possible. The national electric power grid is crumbling under yet another daily scheduled blackout.

If you have no strong objections, I also would like to merge this branch.

How can there be any objections? Please, carry on as you see fit. As previously indicated, I simply lack the expert overall grasp you have on this work, as well as your huge frame of reference. Preferably, I should just stick to the basics of testing the releases you want me to. Still am very grateful to learn from you, though.

@Borewit Borewit linked an issue Feb 5, 2023 that may be closed by this pull request
@Borewit Borewit merged commit b25b9e7 into main Feb 5, 2023
@Borewit Borewit added the debt Technical debt label Feb 8, 2023
@Borewit Borewit deleted the refactor-playlist-writers-with-logging branch February 11, 2023 16:19
@Borewit
Copy link
Owner Author

Borewit commented Feb 17, 2023

Part of release v2.6.0

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

Successfully merging this pull request may close these issues.

Empty line in M3U results in a an empty playlist entry Respect relative path in playlist when it is fixed
2 participants