-
Notifications
You must be signed in to change notification settings - Fork 94
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
gscan: information on hover. #1884
gscan: information on hover. #1884
Conversation
222351f
to
4a8aee2
Compare
This looks really good, but the tooltip information can be out-of-sync with the main state summary display, because the former is retrieved on-demand and the latter automatically at intervals. So, do we need to do one of:
The second option is probably the right thing to do, otherwise if the suite state has changed since last update you'll retrieve a tool-tip for a different state than intended. |
I'd opt for the latter as well. |
4a8aee2
to
e8c7789
Compare
@hjoliver Task information is now obtained along with the main update procedure so as to keep everything in sync. |
Looks good now. |
I'd rather this didn't grab the state_summary on every update, but just on demand - what do you think? |
(over-the-desktop) - Ben's worried about the additional network traffic, and the volume of state summary information. But ... on-demand will necessarily be inconsistent with the summary display sometimes. What about:
I like 2. Note that state summary information can be denied at "public" access level (once gscan follows the CLI scan functionality, to optionally show other users' suites). |
#1887 requires state summary information to be acquired on update, also if the user is skimming across the status indicators then if acquiring on-demand a lot of requests will be generated. Acquiring data on update seems sensible to me, if so are the network traffic issues still a concern? |
@oliver-sanders - I think info on update, not on-demand, is required for consistency. But rather than retrieving the whole suite state summary we should extend the |
6649afb
to
003cb78
Compare
@hjoliver @benfitzpatrick 003cb78 acquires tooltip information along with suite.identify(). |
One suggestion: if the summary ends with "And 1 more" we should just replace that string with the actual result since it takes up the same amount of screen space. |
003cb78
to
d0af55f
Compare
@hjoliver Good point, have implemented. |
@@ -981,6 +1035,10 @@ def update(self): | |||
) | |||
title = suite_info.get("title") | |||
|
|||
if 'tasks-by-state' in suite_info: | |||
self.tasks_by_state[suite + host] = suite_info[ |
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.
Probably better to use (suite, host) as the key for tasks_by_state
, as elsewhere.
Looks good! One comment above, and can we have the numbers of tasks in each state text back in? |
@benfitzpatrick The task state summary text is still there, hover over the point string to see it, if the suite is collapsed then the point string is only a few pixels wide and it is a little hard to hover over. I could change the tooltip text to:
or:
to better incorporate this information. |
Sorry for the delay - yes, could we have the second one? That looks good. |
3ed3ae7
to
53b6caa
Compare
@benfitzpatrick Changed tooltip text to the second proposed form in 53b6caa. |
53b6caa
to
1fe2f79
Compare
I checked forward and backward compatibility, and nothing untoward happens, but for new gscan + old suite, the tooltip says "could not get info, try refreshing the scanner". Is it actually possible for the info to not be supplied until a refresh, or can we assume it must be an older suite daemon and say something like "could not get info; suite running at older cylc version?" |
1fe2f79
to
91554a1
Compare
@hjoliver I don't think it is possible for the info to be missing, I just put the exception in for safetys sake. Have updated the text to "could not get info; suite running at older cylc version?". |
91554a1
to
873e4ee
Compare
Thanks, all good now from my perspective. |
Yep, looks good. |
Just bunging it through the test battery for safety's sake... |
I think the failures are the ones that Matt fixed in #1917. |
Closes #1244
Added tooltip text for the status indicators in gscan. The 5 most recent tasks are displayed, ordered by the most recent of
submitted_time_string
,started_time_string
andfinished_time_string
. If the row is displaying tasks at a given cycle point the tooltip will show only tasks at that point.@hjoliver Please Review
@benfitzpatrick Please Review