-
Notifications
You must be signed in to change notification settings - Fork 3
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
Show progress bar while geometry cache is being loaded #2
Conversation
…ntend to backend in a fixed time interval. Edited CMakeLists.txt to only capitalize the first character of CMAKE_BUILD_TYPE. Ensured semicolon consistency in script.js.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, this works very well and already looks great. We noticed some minor things (see individual comments).
src/qlever-petrimaps/GeomCache.cpp
Outdated
if (_totalSize == 0) { | ||
return 0.0; | ||
} else { | ||
return (static_cast<double>(_curRow) / static_cast<double>(_totalSize) * 100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better if _curRow was now a std::atomic<size_t>
, as concurrent read/writes can now happen.
@@ -1108,6 +1112,18 @@ util::http::Answer Server::handleExportReq(const Params& pars, int sock) const { | |||
return aw; | |||
} | |||
|
|||
util::http::Answer Server::handleLoadStatusReq(const Params& pars) const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When testing the app, I noticed that the percentage often "jumps back" to something like 60% shortly after it has reached 100%. I am not sure why this is happening, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I just noticed that you already gave an explanation for this above :) The parseId
stage is generally quite short, maybe we could cheat a bit and use the first 95% for the geometry progress, and the last 5% for the IDs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is no value of telling the user in which stage of loading they currently are and there won't be any additional stages in the future I think that would be fine. Or maybe it is nice for debugging to let the user know in which stage they were if something goes wrong?
text-align: center; | ||
font-size: 15px; | ||
line-height: 30px; | ||
color: white; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The color contrast is a bit bad here, maybe use color:black
? You could also make the filled bar black, the empty bar white, and set the text color to white
with mix-blend-mode:difference
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second option you provided gives a nice effect, I am just not sure whether a black loading bar is odd to the user. I will keep trying around a little.
width: 0%; | ||
height: 30px; | ||
background-color: #04AA6D; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We experimented with adding a transition: width 500ms
here, this gives a nice effect.
Thanks a lot for this! When you merge this, please take care of two things:
|
into one total progress, displays current stage to the user.
@patrickbr I addressed the issues you mentioned in a new commit:
Are there still things that should be adjusted? |
These commits implement a progress bar while the geometry cache is being loaded.
The frontend sends an HTTP request in fixed time steps to the server in order to get the current progress.
If a request fails or the geometry cache was successfully loaded the frontend stops asking.
In order to process the new
/loadstatus
command aGeomCache
has to be instantiated. Thus far the only implementation of instantiating aGeomCache
Server::loadCache
also started loading the data, so I split the method intoServer::createCache
andServer::loadCache
to be able to only ensure aGeomCache
exists when the loading status is being requested.The loading bar will go from 0% to 100% twice because of the two stages caused by
GeomCache::parse
andGeomCache::parseIds
which reset the progress. This info could also be displayed to the user.