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

Replace search progress dialog with an in-place progress bar #509

Merged
merged 2 commits into from
Sep 11, 2024

Conversation

vifactor
Copy link
Collaborator

@vifactor vifactor commented Jul 26, 2024

From UX point of view, search progress bar dialog detached from main window is one of the most annoying things for me in the application. With this PR, I hide the input dropdown and replace it with a progress bar (+ cancel button) while search is in progress as demonstrated below:

image

#453

@vifactor vifactor force-pushed the search-progress-bar branch 2 times, most recently from f8bee01 to 20e1910 Compare July 26, 2024 16:10
@alexmucde
Copy link
Collaborator

@vifactor There is already a progress bar at the right bottom. Could you use this one instead of a new one?
It is used for load DLT files.
Abort button must be also added there.
For Importing and Exporting it is also used now, but not updated yet, because these functions are still running in main UI Thread. Need to implement new threads for Import and Export.

@alexmucde alexmucde added this to the Release v2.27.0 milestone Jul 30, 2024
@vifactor
Copy link
Collaborator Author

If I remember correctly (no access to the code ATM), the progress bar at the right bottom is used to display indexing progress. The indexing is run in a separate thread, while search is run in the main thread, so they (can) happen concurrently/in parallel. If it is the case, I'm not sure we can combine both progress reports in a single bar.

@alexmucde
Copy link
Collaborator

A signal/slot concept is used to separate the access to the progress bar. What we must achieve that no of the parallel running threads can be started together. So only indexing, search, import or export can be started at once. I will try to find a solution if i find some time.

@vifactor
Copy link
Collaborator Author

vifactor commented Aug 8, 2024

@alexmucde

What we must achieve that no of the parallel running threads can be started together.

sorry, I do not understand that... If computational tasks are "paralellizable", everybody is trying to do the opposite: run them in parallel, in order to utilize all available resources.

Let us perform the following steps:

  1. import relatively big dlt-file
  2. immediately start a search

Currently, we will observe that search progress is reported in a detached dialog, while in the mean time in the bottom right corner, the indexing progress is reported. Search triggered by a user and dlt indexing are independent (hence trivially parallelizable) tasks, whose progress is reported in two different progress bars. This my PR does not change anything in that regard, except that instead of reporting search progress into a detached dialog, it reports progress into a specially crafted progress bar as shown in the screenshot below:
image

A signal/slot concept is used to separate the access to the progress bar.

sure, signal/slots with Qt::AutoConnection + event loop usually do the right things if signals emitted in one thread and slots are executed in another one, but this is not a concern here. As I mentioned above, IMO reporting progress of the two operations into two different bars is the right thing to do here.

@alexmucde
Copy link
Collaborator

My idea was to have only one operation running at once, and so using only one common progres bar for all. Concurrency is nice, but the SW is currently not fully designed for this. Sometimes it will crash at the moment.

@vifactor
Copy link
Collaborator Author

vifactor commented Aug 14, 2024 via email

@alexmucde
Copy link
Collaborator

Ok i will test your implementation. Integration into a single progress bar can be also done later.

@alexmucde
Copy link
Collaborator

@vifactor There is a height issue with the Toolbar, it is too high:
image
Can you fix this?

@vifactor
Copy link
Collaborator Author

vifactor commented Sep 9, 2024

There is a height issue with the Toolbar, it is too high

interesting, I did not observe such thing on my machine. I'll try to understand why it is so, thanks for testing

@alexmucde
Copy link
Collaborator

Great, works now, will merge.

@alexmucde alexmucde merged commit 81a8ef8 into COVESA:master Sep 11, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants