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

Improve tablemodel notification repair playlist #202

Merged
merged 1 commit into from
Sep 22, 2023

Conversation

Borewit
Copy link
Owner

@Borewit Borewit commented Sep 18, 2023

  • Prevent background workers in the playlist-repair-process from calling fireTableDataChanged() directly. This function should only be called by the GUI-Thread. This can prevent race conditions when repairing the playlist.
  • Reduces the number of calls to update the table, when preceding call is pending, the repair mechanism continues
  • Also refactored the code of the repair process a bit

Code changes:

  • Moves PlaylistTableModel.class out of PlaylistEditCtrl.class, to it own file.

Test criteria
Please review and merge #201 first, as that this one is put on top of that.

Expected difference:

  1. The playlist table will updated less progressive
  2. Repairs are performed slightly faster
  3. Possible errors may now be prevented but that is very hard to test for. In following PR, Parallel processing repairing playlist #203, I will propose parallel processing, in that scenario weird errors are more likely to occur without his fix.

@Borewit Borewit self-assigned this Sep 18, 2023
@github-actions
Copy link

@Borewit
Copy link
Owner Author

Borewit commented Sep 18, 2023

@touwys, please note that github-action bot is pointing out the location the distribution, see previous message. If there are multiple messages, I pushed changes, last message reflects the latest change in the PR.

@Borewit
Copy link
Owner Author

Borewit commented Sep 18, 2023

This PR currently introduces a problem. It crashed on a playlist with a few broken entries

@Borewit Borewit marked this pull request as draft September 18, 2023 18:35
@Borewit Borewit force-pushed the improve-tablemodel-notification-repair-playlist branch from 132241e to 8a529e2 Compare September 18, 2023 18:39
@github-actions
Copy link

@Borewit Borewit force-pushed the improve-tablemodel-notification-repair-playlist branch from 8a529e2 to 83f73dc Compare September 21, 2023 16:48
@Borewit Borewit force-pushed the improve-tablemodel-notification-repair-playlist branch from 83f73dc to 47da47f Compare September 21, 2023 16:49
@github-actions
Copy link

@github-actions
Copy link

@Borewit Borewit marked this pull request as ready for review September 21, 2023 17:04
@Borewit
Copy link
Owner Author

Borewit commented Sep 21, 2023

@touwys can you review this one?

@touwys
Copy link

touwys commented Sep 21, 2023

@touwys can you review this one?

👍🏻 Can do tomorrow, DV.

Is this what you want me to look out for?

Expected difference:

The playlist table will updated less progressive
Repairs are performed slightly faster
Possible errors may now be prevented but that is very hard to test for. In following PR, 

Parallel processing repairing playlist #203, I will propose parallel processing, in that scenario weird errors are more likely to occur without his fix.

@Borewit
Copy link
Owner Author

Borewit commented Sep 21, 2023

Is this what you want me to look out for?

Most important is that I did not break anything. If you see difference as described then that is line with expectation, if you see no difference, that is totally fine as well.

Aiming to make a stable release available, without bells and whistles, shall we release after this one, or are there ugly things pending?

@touwys
Copy link

touwys commented Sep 21, 2023

Aiming to make a stable release available, without bells and whistles, shall we release after this one, or are there ugly things pending?

No, there is nothing ugly that is pending that I am aware of. From my point of view, you have taken care of the more detrimental current issue with the fix for #194 and #196. I am going to do another test run tomorrow with the one you released today, to see whether I notice anything untoward, and I will report back as soon as I can.

@touwys
Copy link

touwys commented Sep 22, 2023

@Borewit

listFix()-2.8.0.12_Windows PR #202

During testing, I did not notice anything substantially different in operation from the previous release, listFix()-2.8.0.10_Windows PR #201.

Whether the following matters, or even is something worth considering, is moot: What I did notice was is that the progress bar of the repair-playlist process dialogue, does not progress smoothly, but hesitantly, until closing. Also, for what it's worth, because it is not based on fact-based observation, I got the impression that the repair process has now slowed down slightly, rather than the opposite.

progress bar

@Borewit
Copy link
Owner Author

Borewit commented Sep 22, 2023

What I did notice was is that the progress bar of the repair-playlist process dialogue, does not progress smoothly, but hesitantly, until closing.

That's a good observation. That's because processes working in the background (the repair process) are not allowed to directly update the progress This is exactly what I have changed., and yes, it looks less good, but this is the way to prevent freezing applications, incomplete graphical updates, crashes and all kind of other rare unpredictable side effects.

@Borewit Borewit merged commit 97613cc into main Sep 22, 2023
4 checks passed
@Borewit Borewit deleted the improve-tablemodel-notification-repair-playlist branch September 22, 2023 12:42
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.

2 participants