Skip to content

Improve tablemodel notification repair playlist#202

Merged
Borewit merged 1 commit into
mainfrom
improve-tablemodel-notification-repair-playlist
Sep 22, 2023
Merged

Improve tablemodel notification repair playlist#202
Borewit merged 1 commit into
mainfrom
improve-tablemodel-notification-repair-playlist

Conversation

@Borewit

@Borewit Borewit commented Sep 18, 2023

Copy link
Copy Markdown
Owner
  • 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
Copy Markdown

@Borewit

Borewit commented Sep 18, 2023

Copy link
Copy Markdown
Owner Author

@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

Borewit commented Sep 18, 2023

Copy link
Copy Markdown
Owner Author

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
Copy Markdown

@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
Copy Markdown

@github-actions

Copy link
Copy Markdown

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

Borewit commented Sep 21, 2023

Copy link
Copy Markdown
Owner Author

@touwys can you review this one?

@touwys

touwys commented Sep 21, 2023

Copy link
Copy Markdown

@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

Borewit commented Sep 21, 2023

Copy link
Copy Markdown
Owner Author

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

touwys commented Sep 21, 2023

Copy link
Copy Markdown

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

touwys commented Sep 22, 2023

Copy link
Copy Markdown

@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

Borewit commented Sep 22, 2023

Copy link
Copy Markdown
Owner Author

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
@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