Skip to content

Conversation

AlexeyPeov
Copy link
Contributor

The images table now does not update all of its contents when compression finishes. It redraws only when you put a cursor over it (1st video). I propose a quick fix, to emit dataChanged on compression finish. 2nd video demonstrates that the issue is gone.

Before fix: https://github.com/user-attachments/assets/0e1ca118-06cd-4cea-93e3-79ac1c2d38d8

After fix: https://github.com/user-attachments/assets/d2844394-e5a5-4766-ab3a-fb94f2201ddd

@Lymphatus
Copy link
Owner

Good catch, I had to watch a few times the two videos to get what you meant 😄
My only concern is that it can affect performance on very large lists. Did you notice anything on that regard?

PS: pipelines failing for unrelated reasons.

@AlexeyPeov
Copy link
Contributor Author

About the pipelines - yeah, I had to build the rust lib manually + remove language translation files from "/resources/languages.qrc" to make the project compile (compiler complained about "circular dependencies"). Kinda fixed that, but decided not to include it in this pr.

Now on the topic of performance - there are 3 ways of thinking about it:

1st (my solution): I figured that updating the table manually on compression finish is exactly what happens when you put a cursor over it (the "CImageTreeModel::dataChanged" fn just triggers HtmlDelegate::paint, the performance impact is insignificant, correct me if I'm wrong). I put a counter on HtmlDelegate::paint, and it measured ~700 repaints on compression stage both before and after the change, I used 25 images.

2nd: do not trigger "dataChanged" from "QFutureWatcherBase::progressValueChanged", and only trigger it in "QFutureWatcherBase::finished", this would result in the same amount of calls of "dataChanged" as was before, but it will make compression visually less responsive (bad from user experience standpoint imo).

3rd (this I hadn't figured out): Somehow have the correct status in CImage::getFormattedStatus when QFutureWatcherBase::progressValueChanged for an image gets called, the would be ideal, but I'm not that familiar with this codebase yet, and if you know the solution please let me know.

TLDR: I decided to take the path of least resistance =)

@Lymphatus Lymphatus merged commit 19778b9 into Lymphatus:main Apr 7, 2025
0 of 2 checks passed
@Lymphatus
Copy link
Owner

Your solution sounds right to me. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants