search

COVESA / dlt-viewer

Diagnostic Log and Trace viewing program
Other
425 stars 240 forks source link

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

Closed vifactor closed 1 day ago

vifactor commented 1 month ago

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

https://github.com/COVESA/dlt-viewer/issues/453

alexmucde commented 1 month ago

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

vifactor commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@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 commented 4 weeks ago

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 commented 4 weeks ago

Yeah, but changing that behavior (running indexing in the main thread) is whole new story... Moreover, it means that user won't be able to start a search (the way he/she can do now) until indexing is not finished. Currently, for my dlt files indexing takes some minutes, which I would spend starring into monitor without being able to do my job.

P.S. I looked into indexing and honestly it has two stages: just indexing and indexing with filters, but since usually I do not have filters when I simply open a dlt file, the second index is identical to the first one. Why it is needed at all?.. But again, this is a separate question. As I said the only thing this PR does: it moves visual element (progress bar) from one place to another for more pleasant UI/UX

alexmucde commented 3 days ago

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

alexmucde commented 3 days ago

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

vifactor commented 3 days ago

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 commented 1 day ago

Great, works now, will merge.