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

Clone the playlist type when creating a new playlist #229

Merged
merged 7 commits into from
Dec 1, 2023

Conversation

Borewit
Copy link
Owner

@Borewit Borewit commented Oct 23, 2023

  • Clone the playlist type when creating a new playlist from another playlist.
  • Fix save-as dialog when file has not been saved
  • Prevent unsaved files from trying to be opened after restart
  • Show full path in tab tooltip
  • Change the default save folder, on Windows, from the users home directory to the "My Documents" folder.

Resolves #193

@Borewit Borewit self-assigned this Oct 23, 2023
@Borewit
Copy link
Owner Author

Borewit commented Oct 24, 2023

I not happy with this one, I am going to split the M3U and M3U8 playlist provider in Lizzy: Borewit/lizzy#88

Repository owner deleted a comment from github-actions bot Oct 24, 2023
@Borewit
Copy link
Owner Author

Borewit commented Oct 24, 2023

Can you review this one @touwys?

@touwys
Copy link

touwys commented Oct 25, 2023

ListFix() 3.0.1-4_8932b93

  1. Resolves the 1st part of Issue when creating and saving a new playlist from a selection in the Playlist Editor #193:

    The file-extension is cloned.

Clone file-extension


To save the new playlist by clicking on the Save button, i.e. the diskette icon on the toolbar, produces an unidentifiable flash of "something" on the screen. It does not produce the expected Save Dialogue box, which usually contains the various saving options such as, for example, the preferred name, the location, nor the desired file extension.

  1. The 2nd part of Issue when creating and saving a new playlist from a selection in the Playlist Editor #193 remains unresolved.

    This part is persistent.

Saving a cloned file


@Borewit
Copy link
Owner Author

Borewit commented Oct 25, 2023

The 2nd part of #193 remains unresolved.

Very true, but let's first get a common understanding what the issue is about: #193 (comment)

@touwys
Copy link

touwys commented Oct 26, 2023

The 2nd part of #193 remains unresolved.

Very true, but let's first get a common understanding what the issue is about: #193 (comment)

@Borewit
Please read my comment at #193 (comment).

@Borewit
Copy link
Owner Author

Borewit commented Oct 26, 2023

Changed the following:

  • Fix save-as dialog when file has not been saved
  • Prevent unsaved files from trying to be opened after restart
  • Show full path in tab tooltip
  • Change the default save folder, on Windows, from the users home directory to the "My Documents" folder.

Repository owner deleted a comment from github-actions bot Oct 26, 2023
@touwys
Copy link

touwys commented Oct 27, 2023

Build run (#6658236018, attempt #1) artifacts

@Borewit

Is this build ready for review yet?

@Borewit
Copy link
Owner Author

Borewit commented Oct 27, 2023

Is this build ready for review yet?

It is.

@touwys
Copy link

touwys commented Oct 27, 2023

Review: listFix() 3.0.1-5 fb88fd9

The "Save as" dialogue with which to save the newly created playlist now displays when the diskette on the toolbar is activated, and it presents the needed options.

Issues:

  1. The newly created playlist does not play when hitting the Play-button in the Playlist Editor. Neither can this playlist be played, once saved. Relative paths are enabled in app "Options" (Alt+O).

  2. If the newly created playlist is given a new name in the "Save As" dialogue, the new name is discarded immediately thereafter, as soon as a new file extension is selected. The user should not be forced to select the file extension first, before changing the file name. (Screen video.)

    (Screen video)


Additional Comments:

  1. For Windows users: please use the "Home" / "Desktop" directory as the default start location instead of using "My Documents". The desktop provides much quicker access to more locations. (Screenshot).

    Windows default directory

  2. If it can be accommodated, it is also very desirable to have support for the option that allows access to the last directory used.


@touwys
Copy link

touwys commented Nov 1, 2023

@Borewit

Did I perhaps miss a build update?

@Borewit
Copy link
Owner Author

Borewit commented Nov 1, 2023

@Borewit

Did I perhaps miss a build update?

Not at all, I am just busy with work.

Repository owner deleted a comment from github-actions bot Nov 6, 2023
Repository owner deleted a comment from github-actions bot Nov 6, 2023
Repository owner deleted a comment from github-actions bot Nov 6, 2023
Repository owner deleted a comment from github-actions bot Nov 6, 2023
Copy link

github-actions bot commented Nov 6, 2023

@Borewit
Copy link
Owner Author

Borewit commented Nov 8, 2023

You may review this one @touwys

@touwys
Copy link

touwys commented Nov 9, 2023

@Borewit

listFix()-3.0.1-7-8948a2b

Although I can recognise some of them, please advise me what specifically it is you want me to review? In the meantime, could you please have a look at the issue below before I proceed to review the rest? It hampers the review work flow to an extent.

  1. Issue playing a select number of tracks of a playlist. This occurs once the repair of the playlist has been executed, and these repairs were saved, and while it is still open in the Playlist Editor (PE).

    STEPS:

    1.1 Open a broken playlist in the Playlist Editor (PE).

    1.2 Click "Repair playlist" (the magic wand on the PE toolbar)

    1.3 Save the repaired playlist. (Each track should display the green "OK" icon next to it).

    1.4 Select any number of tracks to play.

    1.5 Click "Play selected" icon on PE toolbar (the green arrow).

    1.6 The selected tracks open as random track numbers in the default player, absent more information, and therefore cannot be played.

    Note: If the playlist tracks are deselected, the whole playlist can be opened in the default player, and can be played.

[Reviewing the cloned playlist "Save as" (#229) dialogue is ongoing]

@touwys
Copy link

touwys commented Nov 9, 2023

listFix()-3.0.1-7-8948a2b

Issues:

  1. After having saved a cloned playlist, and then returning to it in the Editor, select a track(s) to play. The track now playing in the default player, does not match the one selected in the playlist still open in the Editor.

  2. When saving a newly cloned playlist, the new file name gets saved without its selected extension (if not manually added to the file name). Screenshot.


Screenshot


If still applicable, the following actions appear to have been fixed — save for the issues given above:

  1. Fix save-as dialog when file has not been saved ✔️

  2. Prevent unsaved files from trying to be opened after restart ✔️

    Note that I am uncertain which "unsaved files" are getting referred to here. Is it the unsaved, newly cloned files, that are still unsaved at the close of app?

  3. Show full path in tab tooltip ✔️ (A very nice enhancement!)

  4. Change the default save folder, on Windows, from the users home directory to the "My Documents" folder. ✔️

    Please change this default directory to the Windows Desktop from which directory all else is more readily accessible, using fewer clicks. In addition, consider adding support, in the dialogue, or at the app options (Alt+O), to start from the last used directory.

@Borewit
Copy link
Owner Author

Borewit commented Nov 10, 2023

  1. The selected tracks open as random track numbers in the default player, absent more information, and therefore cannot be played.

I do not understand what this means, other then that you cannot play the tracks.
I neither can reproduce the issue, works just fine for me.

@Borewit
Copy link
Owner Author

Borewit commented Nov 10, 2023

Although I can recognise some of them, please advise me what specifically it is you want me to review?

  1. If the issues which the PR claims to have resolved, are resolved.
  2. If no side effects are introduced.

The fix save-as dialog, I had, to kind of hack, the original behavior of this component. I may break under different look & feel (if the look and feel implementation of the JFileChooser does not implements BasicFileChooserUI).

@Borewit
Copy link
Owner Author

Borewit commented Nov 10, 2023

  • After having saved a cloned playlist, and then returning to it in the Editor, select a track(s) to play. The track now playing in the default player, does not match the one selected in the playlist still open in the Editor.

I cannot reproduce that, work just fine for me.

I made very small change which prints the path of the generated playlist at "INFO" level, meaning it now appear by default in you log file.

Can you provide an example, of the playlist (copy the content of the playlist) to an issue, plus the generated playlist which is generated when you hit the play button. You can find the path of the generated playlist in the log file, if you use the build following after this comment.

  • When saving a newly cloned playlist, the new file name gets saved without its selected extension (if not manually added to the file name). Screenshot.

After you replaced the suggested filename with name without an extension? That is the only way I found to reproduce that.

@touwys
Copy link

touwys commented Nov 16, 2023

@Borewit

When you put a new listFix() build up for review, it would be most helpful when you put the instruction for the review right here at the link. Mark it for @touwys and put the salient details along with it. Screenshot example:

image

@Borewit
Copy link
Owner Author

Borewit commented Nov 17, 2023

I think you are underestimating the size of the portable software audience.

Maybe that is indeed interesting audience to include. But I want to keep things very simple. I want to avoid we move configuration files to a different location in favor of the portable deployment, and then run into trouble with the installer.

The ideal deployment in my onion is via an app store, in which users can pull and remove the software with one click.

I have yet to see a warning triggered on one of the portable packages I have had installed over decades of utilising multiple installations of PC software.

You will see such a warning when you use a recent out-of-the-box version of Windows:

!

The only way to proceed, is clicking on "more info" and then you are able to override.

When you put a new listFix() build up for review, it would be most helpful when you put the instruction for the review right here at the link. Mark it for @touwys and put the salient details along with it.

The Build message is being generated by a bot, for your convenience to easier link to the corresponding build. I will ask your review in separate message. The process, including what to review is, consistent:

If will write a message, like I always do, when the PR is worth testing. But I am not going to repeat all obvious information.

  1. A PR is ready for review, unless it is set to Draft
  2. What is be reviewed is described in the first comment and if applicable the context of the issue it tries to resolve, can be taken into account as well. The issue is the problem or request, the PR is the solution.
  3. Which version to test? You can always take the latest build of the PR.

If problems introduced by the PR, should be reported and address in the PR. Problems found which already exist before the PR should be reported in an new issue. If unsure, you can compare behavior with latest main branch build.

This is how we should work: #237 . It is that simple.

- Prevent unsaved files from trying to be opened after restart
- Show full path in tab tooltip
- Change the default save folder, on Windows, from the users home directory to the "My Documents" folder.
- Remove capitals on each word
Preserve the filename the user typed in, when he changes the playlist type (extension).
@Borewit
Copy link
Owner Author

Borewit commented Nov 17, 2023

@touwys, in the next build you have:

  1. Windows zipped distribution (shared config files!)
  2. Build number in each artifact name

For more details see merged PR #241

Copy link

Copy link

@touwys
Copy link

touwys commented Nov 19, 2023

Draft #229 Clone the playlist type when creating a new playlist by Borewit

1. Windows zipped distribution (shared config files!)

2. Build number in each artifact name

For more details see merged PR #241

@Borewit:

What a very pleasant surprise you pulled with this move, thank you! Has it been a difficult exercise so far? I am certainly looking forward to the very first portable release.

Repository owner deleted a comment from touwys Nov 19, 2023
@Borewit
Copy link
Owner Author

Borewit commented Nov 19, 2023

@Borewit : Should I put the finished review here?

Yes, and nothing else please! You generate so much distraction that you have no idea any more what the PR is about.

Please read the following very careful, this is the very minimum you have to understand, otherwise I only spent my time de-tangle the chaos you create.

A PR (Pull Request) is proposed change of the software, typically addressing an issue.
What the PR change is about is summarized in the subject and in the first comment.

So if you test a PR, you test that change. Do not direct the conversation on other subjects, neither on issues already found. At most you mention you came a cross it, and refer to issue you discuss the problem in full detail.

Which build I test?

The last one I published on the PR.

In principal I keep working on the PR until all problems are resolved, and the issue is resolved. When new build is there you can try again and see if previous errors are gone.

Problems which are not introduced by the PR should be reported in a new issue, and addressed in a different PR.

For new issues, it best to take a build from the main branch (and not rely on PR build, which is still subject to change).

@touwys
Copy link

touwys commented Nov 21, 2023

Build run (#6913328926, attempt #1) artifacts

@Borewit

Please view the screenshot. The zip-file and executable it contains, are named after different version numbers.

image

@touwys
Copy link

touwys commented Nov 21, 2023

Build run (#6913328926, attempt #1) artifacts

@Borewit

I installed "listFix-windows-binaries-v2.9.0-22-g83fd38c" in the root directory of the system drive, assuming it is a fully self-contained, portable installation. If it is, it started straight off with the correct app configuration, which it harvested from, and shares with, the current, non-binary, installation. It is therefore not a fully self-contained installation, or is that still in the pipeline?

image

@Borewit
Copy link
Owner Author

Borewit commented Nov 21, 2023

Please view the screenshot. The zip-file and executable it contains, are named after different version numbers.

Oh this is very confusing. The short answer is, either build reflect the last change. The versions are a mess. And the build is actually not one the last commit, but potential merge with the main branch.

v2.9.0-22-g83fd38c is the correct one, as the commit distance is 22, 15 is bogus.

I will spawn it to a different issue. I will also put a versioned sub-folder inside the versioned "portable build".

@Borewit Borewit merged commit 41dd309 into main Dec 1, 2023
4 checks passed
@Borewit Borewit deleted the clone-playlist-type branch December 1, 2023 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue when creating and saving a new playlist from a selection in the Playlist Editor
2 participants