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

Support for multiple playlist folders #93

Merged
merged 15 commits into from
Feb 17, 2023

Conversation

Borewit
Copy link
Owner

@Borewit Borewit commented Feb 12, 2023

Changes:

  1. Add support for multiple playlist folders
  2. Removed are you sure dialog before showing renaming dialog
  3. Keep playlist-editor-tabs in sync with playlists-directories
  4. Disabled:
    1. Renaming multiple files
    2. Renaming playlists

Resolves #68, #55, #91

@Borewit Borewit added the enhancement Request or implementation of new feature label Feb 12, 2023
@Borewit Borewit self-assigned this Feb 12, 2023
@Borewit Borewit force-pushed the support-multi-playlist-directories branch from 7224c39 to 18f65db Compare February 12, 2023 17:37
@Borewit
Copy link
Owner Author

Borewit commented Feb 12, 2023

Build: listFix_2.5.1-PR93-28.exe, commit 18f65db

@Borewit Borewit force-pushed the support-multi-playlist-directories branch from 18f65db to 5f0fb1b Compare February 12, 2023 21:02
@touwys
Copy link

touwys commented Feb 13, 2023

Build: listFix_2.5.1-PR93-28.exe, commit 18f65db

I am onto it, today. 😅

@touwys
Copy link

touwys commented Feb 13, 2023

Build listFix_2.5.1-PR93-28:

Initial Test Steps:

  1. Deleted "rollingfile.log".

  2. Installed Build listFix_2.5.1-PR93-28 over the previous build.

  3. Start up:

3.1 Media Directories are missing in display. (Screenshot).

3.2 Playlist Directories are on display. (Screenshot).

  1. Additional Playlist Directory added 100%, including subdirectories. Button layout looks good! (Screenshot).

  2. Open 12 x playlists for repair in the Playlist Editor (PLE), simultaneously, from the newly added playlist directory.

5.1 The 12 tab-titles of the playlists (5) were still marred by "double-printing", effectively handicapping the repair process. However, we may, at last, be onto something? — See 5.2. (Screenshot).

5.2 Even though the Media Directory was empty, playlists could still be repaired — because the closest matches for the missing were still getting acquired. (Screenshot).


Screenshot: Build listFix_2.5.1-PR93-28

rollingfile.log

@touwys
Copy link

touwys commented Feb 13, 2023

Even though the Media Directory was empty...

Should we post a separate issue for this item?

@Borewit
Copy link
Owner Author

Borewit commented Feb 13, 2023

3.1 Media Directories are missing in display. (Screenshot).

I renamed a method internally, by doing that I forgot it also effected the configuration file format. I will fix that. Good catch.

Button layout looks good!

I don't think I have changed anything, other then renaming the button from "Set" to "Add".

5.2 Even though the Media Directory was empty, playlists could still be repaired — because the closest matches for the missing were still getting acquired. (Screenshot).

Ah yes, all the underlying directories and files are cached in the configuration. As those were still present, the repair process still works.

I moved the media directories from options.json to mediaLibrary.json which was a bad idea, as the mediaLibrary.json is very expensive to write as it contains all the media files.

@Borewit
Copy link
Owner Author

Borewit commented Feb 13, 2023

Should we post a separate issue for this item?

No, this PR is not merged (still open). As the issue is introduced with this PR, the way to deal with with it is to resolve it in this PR.

@Borewit Borewit force-pushed the support-multi-playlist-directories branch from 5f0fb1b to 63c42fc Compare February 13, 2023 10:48
@Borewit
Copy link
Owner Author

Borewit commented Feb 13, 2023

This build listFix_2.5.1-PR93-31.exe should resolve some side effects introduced with the multi playlist directories support:

  1. It should respect the old configuration from previous version and migrate the single playlist directory to the multi playlist directories.
  2. No longer show up with no playlist directories.

If you use the config files from listFix_2.5.1-PR93-28 I expect you will loose the multi playlist directories list defined in that one.

@touwys
Copy link

touwys commented Feb 13, 2023

Testing Build: listFix_2.5.1-PR93-31

This build was installed over build listFix_2.5.1-PR94-30.

  1. It should respect the old configuration from previous version and migrate the single playlist directory to the multi playlist directories.

— It did not.

2. No longer show up with no playlist directories.

— Did you perhaps you mean Media Directories?

— It also did not display the additional Playlist Directory that I added in listFix_2.5.1-PR94-30.

If you use the config files from listFix_2.5.1-PR93-28 I expect you will loose the multi playlist directories list defined in that one.

— Do you want me to go back and install this build again? I still do not know in what the specific sequence is you want me to install the builds?

Note the screenshots: Double-printed tab-titles are back? 😥


listFix_2.5.1 [PR93]-31: Double-printed tab-titles

rollingfile.log

@Borewit Borewit force-pushed the support-multi-playlist-directories branch from d97ad8b to 770c6d7 Compare February 13, 2023 19:49
@Borewit
Copy link
Owner Author

Borewit commented Feb 14, 2023

Okay @touwys, I have done another drastic round of refactoring listFix_2.5.1-PR93-36.exe, changed in total 1800 lines of code 😓.

There is a high probability I broke some things, but there is also a high probability I fixed the infamous #55 issue.
I disrespected the scope this PR, you can do the same with testing.

  1. Did I fix Double printed tab pane headings of the playlist editors #55?
  2. Did I break something new?
  3. Is (are) the playlist Save as working well?
  4. I changes code related to copy paste, and actually don't even now how that should work.

@touwys
Copy link

touwys commented Feb 15, 2023

Okay @touwys, I have done another drastic round of refactoring listFix_2.5.1-PR93-36.exe, changed in total 1800 lines of code.

@Borewit :

👍🏻 Wow, you broke a lot of sweat over this one. May it deliver the expected results. Please tell me whether you took the results of #94 (comment) into consideration when you rewrote all that bunch of code — because the "double-printed" tab-titles were "fixed" in that build?

There is a high probability I broke some things, but there is also a high probability I fixed the infamous #55 issue.

#55 was "fixed"? See: #94 (comment)

I disrespected the scope this PR, you can do the same with testing.

🤔 Uncertain what you mean by this. Would you, please, elaborate?

@Borewit
Copy link
Owner Author

Borewit commented Feb 15, 2023

I only found your comments after I did the code rewrite. But the versions in #94, are not real solutions.

  • listFix_2.5.1-PR94-30 fixed the double printed header, by disabling functionality, which is the close button on the tabs. But it helped to confirm my suspicion that this was causing the problems you experienced.
  • listFix_2.5.1-PR94-31 I renabled the close button, and made a small change (added the validate()), but I strongly doubt that this one really fixes the issue. You also reported the problem from reoccuring here. What neither works in that version, is renaming the tab via Save as. And I suspect that is actually what was really the source of the issue. The title and icons of the tab are overwritten, but it wired to the default tab layout, which I think was reviving the old tab layout. For some reason only on you machine so far. I suspect when you save the playlist as a different file, you can trigger the double headings.

🤔 Uncertain what you mean by this. Would you, please, elaborate?

Scope of this PR was Support for multiple playlist folders and I ended improving / refactoring code in a lot's of area's.

Ideally we maintian software in ordered way (the boxes). Let's pretend this software is a car. If there is an issue with the exhaust, we fix the exhaust. We don't give it new tires, bigger navigaton screen etc. That way the tester knows he can just focus on the exhaust... Yet this code is far from perfect, so requires some drastic iterations to get in into shape.

So the ideal flow workflow should be like:

Issue (broken exhaust) ➽ PR/branch we try to fix the exhaust, on a branch (~copy) of the car ➽ Test PR (the repaired exhaust) until okay ➽ Merge: we put repaired exhaust fix on the real car (which is the same as, we replace the car with the fixed copy of the car).

@touwys
Copy link

touwys commented Feb 15, 2023

Testing: listFix_2.5.1-PR94-31

#93 (comment)

I read your reply above only after completing the initial testing of Testing: listFix_2.5.1-PR94-31:

Did I fix?

See the "boxed" steps in testing listFix_2.5.1-PR93-36, below — especially (6.1). Hopefully, these will give you the answers you're looking for. Let me know if it doesn't, and I will continue upon receiving your follow-up instructions.

I changes code related to copy paste, and actually don't even now how that should work.

Are you referring to PL-Tab / Context Menu / "Copy Selected Items To..."?. If you are, I did test that, and the action copies the selected files to the folder of your choice.

____________________________________________

TESTING: listFix_2.5.1-PR93-36
____________________________________________

  1. Deleted log file: listFix_2.5.1-PR94-31

  2. Installed listFix_2.5.1-PR93-36.

ℹ️ 2.1 The json configuration files of listFix_2.5.1-PR94-31 were retained for the test.

  1. Started listFix_2.5.1-PR93-36.

ℹ️ 3.1 Media & Playlist Directories from the previous installation are displayed.

  1. Add new Playlist Directory (PLD). These are the Actions performed on the Newly Added PLD, with the results — :

ℹ️ 4.1 PLD (Folder) / Context Menu / "Remove Playlist Directory" — =fail.

ℹ️ 4.2 PLD (Folder) / Context Menu / "Rename" — pass. (This action invokes the intermediate "Rename Selected Files & Folders?" dialogue.)

ℹ️ 4.3 Rename Playlist (PL) after the PLD name change — =fail.

ℹ️ 4.4 Rename the PLD — back to its original name.

ℹ️ 4.5 Rename PL in PLD — =pass.

  1. Switch focus to the "Kept" Playlist Directory (PLD). These are the Actions performed on the kept PLD, with the results — :

ℹ️ 5.1 Rename PL — =pass.

  1. Next, opened a number of PL's from both the "kept" PLD and the "added" PLD into the Playlist Editor (PLE). Results — :

ℹ️ 6.1 Congratulations & jubilations! The tab-titles display without the layered text/"double-print".

ℹ️ 6.2 New, weird thing spotted with tab-titles: Clicking on the tab-title (the text part), fails to switch focus to the selected tab. However, clicking to the side of the tab-title (the part void of text) does switch the focus to that tab.

  1. File / Menu / Save As (Ctrl+Shift+S) — =pass. (Is the "Save As" action available from elsewhere in the app?)

  2. Close all PL-tabs in PLE.

  3. End test.

  4. Close app and report.


rollingfile.log = Zero data? 🤔

@Borewit
Copy link
Owner Author

Borewit commented Feb 15, 2023

Thanks or testing @touwys. I am very glad #55 is history.
I did my best to resolve the issues you reported on.

Build: listFix_2.5.1-PR93-40.exe

Please let me know if I missed something, or left something broken @touwys.

@Borewit Borewit linked an issue Feb 15, 2023 that may be closed by this pull request
@touwys
Copy link

touwys commented Feb 15, 2023

@Borewit:

Thanks, I have to go off-line just now.

Downloaded, and did one quick test with Build listFix_2.5.1-PR93-40, if you wish to take another look at the issue in the meantime:

ℹ️ — Clicking on a Playlist Tab, does not change the focus to the selected tab.

@Borewit
Copy link
Owner Author

Borewit commented Feb 15, 2023

ℹ️ — Clicking on a Playlist Tab, does not change the focus to the selected tab.

FIxed in this one: listFix_2.5.1-PR93-41.exe

@Borewit Borewit force-pushed the support-multi-playlist-directories branch from bd063e4 to 0488e49 Compare February 15, 2023 21:55
@Borewit
Copy link
Owner Author

Borewit commented Feb 15, 2023

Previous build fails to activate the tab after the playlist has been renamed.

Fixed that one: listFix_2.5.1-PR93-42.exe

@touwys
Copy link

touwys commented Feb 16, 2023

Testing Build: listFix_2.5.1-PR93-42

Configuration files and log from build listFix_2.5.1-93-40 were retained.

Herewith, a brief summary of test results:

  1. Steps: Click on a PL-tab in PLE to select, and focus on it.

ℹ️ 1.1 Fixed: Clicking on the PL tab-title, now do change the focus to the one selected.

  1. Steps: Select PLD / Context Menu / Rename:

ℹ️ 2.1 The "Rename" action is now greyed out. PLD's thus unavailable for renaming?

  1. Steps: Select PLD-subfolder (-directory) / Context Menu / Rename:

ℹ️ 3.1 PLD subfolders/directories: "Rename" — is not greyed out. Can be renamed.

  1. Steps: Select PL anywhere in expanded PLD / Context Menu / Rename:

ℹ️ 4.1 PL's in the PLD's can be renamed.

  1. Steps: Select (a current, previously configured folder) PLD / Open multiple PL's from the PLD into PLE / Rename PL's in the PLD:

ℹ️ 5.1 At first, there were no correlation between the PL tab-titles now open in the PLE, and the PL-names which are displayed in the PLD. The relationship between the PL-names shown in the PLD, and the PL-names displayed in the PLE tabs, were not maintained to mirror each other. However, for some reason, after closing the app and restarting it, this name-relationship was correctly maintained during the renaming process?

  1. Steps: Select PL in PLD / Context Menu / Delete:

ℹ️ 6.1 Deleting a file located in the PLD, does not remove/clear the corresponding PL/tab in the PLE.


listFix_2.5.1-PR93-42: rollingfile.log

@Borewit
Copy link
Owner Author

Borewit commented Feb 16, 2023

@touwys

The "Rename" action is now greyed out. PLD's thus unavailable for renaming?

I did change that behavior indeed. My initial aim was to remove the question, do you really want to rename?

I think that initial dialog was in place as possibly the renaming could apply to multiple files, go in through potentially a long loop of opening dialogs.

Well, I think it more effective to only allow a playlist to be renamed. Hence I disabled the option to rename if more then one playlist is selected.

In addition to that, I disabled renaming the playlist folder, it could trigger all sorts of side effects.

At first, there were no correlation between the PL tab-titles now open in the PLE, and the PL-names which are displayed in the PLD. The relationship between the PL-names shown in the PLD, and the PL-names displayed in the PLE tabs, were not maintained to mirror each other. However, for some reason, after closing the app and restarting it, this name-relationship was correctly maintained during the renaming process?

My aim is to keep tab name in sync with the playlist filename. So renaming a playlist filename, should result in a change in filename in the tab.

@Borewit
Copy link
Owner Author

Borewit commented Feb 16, 2023

6.1 Deleting a file located in the PLD, does not remove/clear the corresponding PL/tab in the PLE.

Fixed that: listFix_2.5.1-PR93-44

@Borewit Borewit linked an issue Feb 16, 2023 that may be closed by this pull request
@touwys
Copy link

touwys commented Feb 17, 2023

Testing: listFix_2.5.1-PR93-44

6.1 Deleting a file located in the PLD, does not remove/clear the corresponding PL/tab in the PLE.

👍🏻 Fixed! This old car exhaust "box" doesn't backfire any more. 🤣


I did change that behavior indeed. My initial aim was to remove the question, do you really want to rename?

ℹ️ Yes, I do. (Also read my comment at your last quote, below.)


I think that initial dialog was in place as possibly the renaming could apply to multiple files, go in through potentially a long loop of opening dialogs.

ℹ️ Yes, I agree — even though the need does arise, and maybe more often so than generally recognized. However, unless it is really easy to implement the multi-rename action, it is not something to focus on — at least not right now. Maybe later on? Regardless, it is quick enough to use an external rename app for this process.


Well, I think it more effective to only allow a playlist to be renamed. Hence I disabled the option to rename if more then one playlist is selected.

ℹ️ I assume by "only allow a playlist to be renamed" you mean that to the exclusion of all folders/directories? If so, please reconsider to reactivate this very useful function to rename the root-folders/directories — if only for the sake of consistency — because sub-folders of the root-folders/directories can still be renamed.


Testing on this build is still ongoing.

@touwys
Copy link

touwys commented Feb 17, 2023

As you suggest, let's capture that in a new issue, and solve that in a separate PR.

Agreed.


I wait for your review. I propose we keep on shaking out all bugs possibly introduced by this PR until nothing falls out the tree. When you give me green light, I merge this PR and I i will release "2.6.0".

This time the testing may take a while longer than usual. Nonetheless, I will continue to report issues — if any — as they surface during use.

@Borewit Borewit force-pushed the support-multi-playlist-directories branch from cbfb91a to 99ca28e Compare February 17, 2023 09:18
@Borewit
Copy link
Owner Author

Borewit commented Feb 17, 2023

Merged PR's

Which I forgot about. I "rebased" this PR on top of those, that's why you see long list of changes before this message.

@touwys
Copy link

touwys commented Feb 17, 2023

Which I forgot about. I "rebased" this PR on top of those, that's why you see long list of changes before this message.

OK, so these are already contained in the build I am testing?

@Borewit
Copy link
Owner Author

Borewit commented Feb 17, 2023

OK, so these are already contained in the build I am testing?

No, will provide you a new build with those + disabling renaming of subfolders

@Borewit
Copy link
Owner Author

Borewit commented Feb 17, 2023

Build: listFix_2.5.1-PR93-49.exe

@touwys
Copy link

touwys commented Feb 17, 2023

Testing — listFix_2.5.1-PR93-49:

  1. Renaming of root & subfolders & multi-playlists — confirm disabled.

  2. Location: PLE: Overlaid text returned to the playlist tabs. It appears different from before — a wider spread of the exclamation mark. (Screenshot)


PL tab-titles: Overlaid text

rollingfile.log


@Borewit
Copy link
Owner Author

Borewit commented Feb 17, 2023

Any improvement with this one? listFix_2.5.1-PR93-50.exe

@touwys
Copy link

touwys commented Feb 17, 2023

Any improvement with this one? listFix_2.5.1-PR93-50.exe

The download from MediaFire appears to be throttled and is failing. Perhaps network connection problems.

If you wish, please upload the file to these sites as well, so I can test them all for speed?

Workupload

Easyupload

Ufile

Thanks.

@touwys
Copy link

touwys commented Feb 17, 2023

Any improvement with this one? listFix_2.5.1-PR93-50.exe

Fixed.

@Borewit
Copy link
Owner Author

Borewit commented Feb 17, 2023

Fixed.

Awesome!

The download from MediaFire appears to be throttled and is failing.

We only use that for intermediate builds, proper build shall be uploaded directly to GitHub.

@Borewit Borewit merged commit d0c102c into main Feb 17, 2023
@Borewit Borewit deleted the support-multi-playlist-directories branch February 17, 2023 16:47
@Borewit
Copy link
Owner Author

Borewit commented Feb 17, 2023

Part of release v2.6.0

@touwys
Copy link

touwys commented Feb 19, 2023

Part of release v2.6.0

Great work, great determination, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Request or implementation of new feature
Projects
None yet
2 participants