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

Display message triggers #530

Merged
merged 15 commits into from
Feb 24, 2021
Merged

Display message triggers #530

merged 15 commits into from
Feb 24, 2021

Conversation

kinow
Copy link
Member

@kinow kinow commented Nov 5, 2020

These changes close #402

  • display v-chip's with the message "labels"
  • display the message text as a tooltip of the v-chip
  • hide the message if its flag (satisfied, received, etc) is not true
  • limit number of v-chip's to 5 most recent (remember to check the backend model for a timestamp...)
  • when more than 5 v-chip's, display an icon showing that we have more; when pressed, open job details
  • also include all the "label" & "message" in the job details (leaf-node)

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Appropriate tests are included (unit and/or functional).
  • Appropriate change log entry included.
  • No documentation update required.

@kinow kinow added this to the 0.3 milestone Nov 5, 2020
@kinow kinow self-assigned this Nov 5, 2020
@kinow
Copy link
Member Author

kinow commented Nov 5, 2020

Long messages are now abbreviated/truncated. It shows the first message text only, accompanied by a number as "(2)", where 2 is the total number of custom messages.

image

Also added a tooltip that displays each custom message in a paragraph. Still a draft, so will improve the code later to remove duplicated code and avoid iterating the arrays multiple times (if possible; due to v-if to check if we will display the component or not...)

image

Current workflow used for testing:

[scheduling]
   cycling mode = integer
   initial cycle point = 1
   [[graph]]
      R1 = """
           foo:out1 => bar
           baz:out1 => bar
      """
[runtime]
   [[foo]]
       script = """
           sleep 20
           cylc message "Aliquam a lectus euismod, vehicula leo vel, ultricies odio."  # <-----
           sleep 20
       """
       [[[outputs]]]
           out1 = "Aliquam a lectus euismod, vehicula leo vel, ultricies odio."
   [[baz]]
      script = """cylc message "Lorem ipsum dolor sit amet, consectetur.""""
      [[[outputs]]]
         out1 = "Lorem ipsum dolor sit amet, consectetur."

@hjoliver
Copy link
Member

hjoliver commented Nov 6, 2020

(Big discussion on Element on how to get the output message in the upstream (emitting) task, just needs a small change on the back end).

@kinow
Copy link
Member Author

kinow commented Nov 10, 2020

Working on changes to start working on the updates for this PR.

First step is to:

  • expand the TaskProxy (will edit commit to remove it later), so that I don't have to always expand a task proxy while developing 😄 lazy, I know, but I will revert it 👍
  • add an array to the Jobs, with custom messages

I'm not worrying about which message is coming from which task/job. Instead, every job will have the same 10 messages, only for developing/testing. I've used the following structure

const messages = [
  {
    'label': 'label1',
    'message': 'message1',
    timestamp: 123456
  },
  {
    'label: 'label2',
    'message': 'message2',
    timestamp: 234567
  },

That should be easy to use in the UI, and we easy to be modified to whichever data model is implemented in the backend 👍

image

(I changed the JS structure, to have label and message attributes, as that's much easier to use in the JS template)

image

@codecov-io
Copy link

codecov-io commented Nov 10, 2020

Codecov Report

Merging #530 (0505b3f) into master (160ca04) will decrease coverage by 33.24%.
The diff coverage is 73.80%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #530       +/-   ##
===========================================
- Coverage   82.80%   49.55%   -33.25%     
===========================================
  Files          66       67        +1     
  Lines        1320     1354       +34     
  Branches       81       81               
===========================================
- Hits         1093      671      -422     
- Misses        208      662      +454     
- Partials       19       21        +2     
Flag Coverage Δ
e2e 49.55% <73.80%> (+0.84%) ⬆️
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/components/cylc/tree/TreeItem.vue 96.15% <ø> (ø)
src/graphql/queries.js 100.00% <ø> (ø)
src/components/cylc/tree/cylc-tree.js 47.68% <16.66%> (-52.32%) ⬇️
src/components/cylc/tree/index.js 90.00% <85.71%> (-10.00%) ⬇️
src/model/TaskOutput.js 100.00% <100.00%> (ø)
src/services/mock/workflow.service.mock.js 93.33% <100.00%> (+5.09%) ⬆️
src/services/user.service.js 0.00% <0.00%> (-100.00%) ⬇️
...ponents/graphqlFormGenerator/components/Object.vue 0.00% <0.00%> (-100.00%) ⬇️
.../components/graphqlFormGenerator/FormGenerator.vue 0.00% <0.00%> (-82.61%) ⬇️
src/utils/aotf.js 10.86% <0.00%> (-77.18%) ⬇️
... and 25 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 160ca04...0505b3f. Read the comment docs.

@kinow
Copy link
Member Author

kinow commented Nov 10, 2020

I'm working on display the messages in the job-details panel. For that I'm also doing something I've been postponing, that's to make job-details a node in the tree, which reduces a bit the logic from the template.

How does this look for now @hjoliver , @oliver-sanders ?

  • Used a divider to separate job details from job messages
  • Also added a message when there are no messages (ignore the actual messages for now 😄 just quick templating hacking for development)

image

  • Displaying the messages

image

The questions I have right now are:

  1. Should we have a divider? Or should we just display everything in a single place/group?
  2. If this is looking OK, should we align the details and messages (see they are not quite the same width, but that can be fixed, just requires more work/time, so I'm asking before doing that to avoid re-work)
  3. Is that OK to display all messages for now? Later we can hide and show only 5 too, and have a button or something that displays all the messages…

Thanks!

p.s.: we have 15 messages in each job now, for testing; there is a flag that is true only if the index of the message is even; we are displaying only the 5 first messages, which means you should always see messages 0, 2, 4, 6, 8 labels in the chips

@hjoliver
Copy link
Member

hjoliver commented Nov 11, 2020

I'm not too concerned about alignment and divider, whatever you think looks best is fine for this PR; we can tweak those things later if necessary.

I see you're not displaying most-recent-message first yet in the chip ordering? (if there are lots of messages, seeing the only the first 5 while the next 95 come in slowly wouldn't be ideal).

We should probably list all the messages in job details, so we can see what hasn't been received yet as well as what has been received, in which case some way of distinguishing the two cases is needed - maybe greyed out for not received?

@hjoliver
Copy link
Member

Also, the latest message field will not be needed anymore, since that will be obvious from the list of messages and the chips. @dwsutherland can you remove that from the schema too?

@kinow
Copy link
Member Author

kinow commented Nov 11, 2020

Ah! Forgot it must be the Nth most recent messages. Will fix in the next commit.

And good idea on displaying pending messages in a different style! Thanks Hilary!

@oliver-sanders
Copy link
Member

oliver-sanders commented Nov 11, 2020

  1. Should we have a divider? Or should we just display everything in a single place/group?

Perhaps a sub-header called "outputs" or "custom outputs".

  1. If this is looking OK, should we align the details and messages (see they are not quite the same width, but that can be fixed, just requires more work/time, so I'm asking before doing that to avoid re-work)

Can we use the same alignment as in the previous (job details) section? That looks pretty good.

  1. Is that OK to display all messages for now? Later we can hide and show only 5 too, and have a button or something that displays all the messages…

We're still in α so anything flies!

We will definitely need to collapse them somehow as (for some niche cases) there will be a large number. 5 sounds like a sensible number.

@kinow
Copy link
Member Author

kinow commented Nov 11, 2020

Here's today update:

  • implemented all features discussed on Riot
  • added a subheader as @oliver-sanders suggested, now it's much easier to locate the outputs section, and also easier for new users to get used to it IMO 👍
  • reverted the expanded tasks so that we have the same old behaviour (will edit commits to remove this change later)

GIFrecord_2020-11-12_104825

The items are now aligned equally, in job details & output messages

image

I've changed the mocked data to return the list ordered by timestamp, with most recent messages first. That's useful for the chips, but I left it in the job details panel too. So you see msg-label-14, msg-label-13, etc. Is this order OK?

What should we do about the mobile/smaller viewports?

image

@hjoliver
Copy link
Member

Looking really good @kinow 👍

I've changed the mocked data to return the list ordered by timestamp, with most recent messages first. That's useful for the chips, but I left it in the job details panel too. So you see msg-label-14, msg-label-13, etc. Is this order OK

If we always displayed the entire list (in job details) it should probably be in the natural order (not reversed like the chips). BUT we intend to truncate that list by default, to say five lines (in this PR, or in a follow-up?), so that would require reverse ordering like the chips to ensure that the most recent messages are prioritized for display. That being the case, it seems to me we'd want:
always reverse order (with and without truncation); OR natural order when expanded and reverse order when truncated - but that would look weird when toggling between expanded and truncated? So on that basis I think we should stick with reverse order!

What should we do about the mobile/smaller viewports?

Can we cut off more chips as the window shrinks, down to a single one, rather than flowing onto the next lines?

Hmm, that suggests we should not have a fixed number like 5, but as many as will fit in the current window size? (Is that easy enough to do?)

@kinow
Copy link
Member Author

kinow commented Nov 12, 2020

@dwsutherland shared this query of what it might look like in the GraphQL implementation. It matches the schema used in this draft PR (thanks a lot David!), so finishing up this PR once we are done with the final adjustments should be fairly simple.

query {
  workflows (ids: ["*|vix"]) {
    tasks (ids: ["foo"]) {
      proxies (ids: ["1|foo"]) {
        latestMessage
        outputs (satisfied: false, limit: 5, sort: {keys: ["time"], reverse: true}) {
          label
          message
          satisfied
          time
        }
        extras
      }
    }
  }
}

@kinow
Copy link
Member Author

kinow commented Nov 12, 2020

Looking really good @kinow +1

Thanks!

I've changed the mocked data to return the list ordered by timestamp, with most recent messages first. That's useful for the chips, but I left it in the job details panel too. So you see msg-label-14, msg-label-13, etc. Is this order OK

If we always displayed the entire list (in job details) it should probably be in the natural order (not reversed like the chips). BUT we intend to truncate that list by default, to say five lines (in this PR, or in a follow-up?), so that would require reverse ordering like the chips to ensure that the most recent messages are prioritized for display. That being the case, it seems to me we'd want:
always reverse order (with and without truncation); OR natural order when expanded and reverse order when truncated - but that would look weird when toggling between expanded and truncated? So on that basis I think we should stick with reverse order!

All good for me 👍

What should we do about the mobile/smaller viewports?

Can we cut off more chips as the window shrinks, down to a single one, rather than flowing onto the next lines?

Hmm, that suggests we should not have a fixed number like 5, but as many as will fit in the current window size? (Is that easy enough to do?)

Good idea! Let me think a bit how to implement this… 🤔

@kinow
Copy link
Member Author

kinow commented Nov 12, 2020

So far the only thing I could think of, is controlling the amount of chips displayed based on the viewport. So small viewport display 1, medium display 3, and bigger ones display 5.

However, this works only when the host name is not really long. Right now it looks a bit busy with so many messages in two side-by-side tree views.

image

The simplest approach would be to display just 1 chip, with either the number of messages, or just the first message label? Or do that in the small viewport, and then show 5 when the viewport is bigger (tablet & desktop sizes)?

@hjoliver
Copy link
Member

The simplest approach would be to display just 1 chip, with either the number of messages, or just the first message label? Or do that in the small viewport, and then show 5 when the viewport is bigger (tablet & desktop sizes)?

I think that would be fine for this PR, and consider improving it later.

@kinow
Copy link
Member Author

kinow commented Nov 15, 2020

The simplest approach would be to display just 1 chip, with either the number of messages, or just the first message label? Or do that in the small viewport, and then show 5 when the viewport is bigger (tablet & desktop sizes)?

I think that would be fine for this PR, and consider improving it later.

Do you mean to display 1 chip, or doing that when the viewport is small, and keep the 5 when the viewport is bigger @hjoliver ?

@hjoliver
Copy link
Member

Actually I'm not sure. I don't think it's great to display only a single chip no matter what the view port. Ideally we need to show as many messages as possible without wrapping the v-chips onto the next line. But 5 most recent messages with wrapping in smaller view ports is probably fine for this PR - would you agree @oliver-sanders ?

@oliver-sanders
Copy link
Member

I would have thought something like displaying the five most recent with overflow-y: none (and an ellipsis at the end of the line in the future).

@hjoliver
Copy link
Member

hjoliver commented Nov 16, 2020

I was wondering if we couldn't get the ellipsis in the right place if using CSS overflow like that. But I suppose it could be in another div on to right of the truncated list of v-chips?

@kinow
Copy link
Member Author

kinow commented Nov 19, 2020

Ellipsis are easier to use with text. With children

//etc it gets trickier.

Tried a few variations today, without luck. Hiding when viewport is small doesn't necessarily solve it. In medium size it may still create extra lines due to label width.

The easiest approach I found was to make it display the job information, including messages, in a single row, without wrapping. Does that look like a possible approach (maybe for now?)?

GIFrecord_2020-11-19_215600

@hjoliver
Copy link
Member

@kinow in @oliver-sanders comment:

I would have thought something like displaying the five most recent with overflow-y: none (and an ellipsis at the end of the line in the future).

I guess by overflow-y:none he means overflow-y:hidden? Wouldn't that cause the chips to split onto following lines, but be hidden, with no scroll bar? Have you tried that?

What about doing that, with max 5 chips, but put the ellipsis (or +) first, in front on the chips? Even if ellipsis first isn't the ideal place to put it, that would provide the necessary interface (most recent chip(s) visible, and ellipsis if there's more, without having to change the number of chips as the window shrinks. That might be the best interim solution, until/if we figure out something better.

@kinow
Copy link
Member Author

kinow commented Nov 19, 2020

I guess by overflow-y:none he means overflow-y:hidden? Wouldn't that cause the chips to split onto following lines, but be hidden, with no scroll bar? Have you tried that?

What about doing that, with max 5 chips, but put the ellipsis (or +) first, in front on the chips? Even if ellipsis first isn't the ideal place to put it, that would provide the necessary interface (most recent chip(s) visible, and ellipsis if there's more, without having to change the number of chips as the window shrinks. That might be the best interim solution, until/if we figure out something better.

I tried that, but couldn't get the ellipsis or indicator of more chips to work. Moving the + sign before the chips could work. I think as long as we are consistent, that should be OK. But if in other elements we have an ellipsis or + after the list, then it would be at least awkward IMO.

How does this look?

GIFrecord_2020-11-20_100758

If you have a long task proxy name (or deeply nested hierarchy for parents), several jobs, or a long host name, the scroll bar might be displayed anyway (repair that the 20000102T0000Z / GOOD / SUCCEEDED / eventually_succeeded) the scroll bar will be activated anyway. So I thought that limiting the number of chips for that case wasn't worth, that's why last evening I tried to show the 5 chips inline.

image

But not sure which approach is better for Cylc users :-) so happy to go with either way.

@hjoliver
Copy link
Member

hjoliver commented Nov 19, 2020

Hmm maybe you're right.

@oliver-sanders - what do you think?

  • shall we just go with 5 chips and a scroll bar for narrow view ports, for now?
  • or the 2nd example above, with overflow:hidden and ellipsis in front

(I actually quite like the 2nd example, but ellipsis in front is a bit quirky).

@kinow
Copy link
Member Author

kinow commented Jan 10, 2021

ping @oliver-sanders ☝️ 😬

@oliver-sanders
Copy link
Member

oliver-sanders commented Jan 11, 2021

The second example is really nice (no idea how it works)!

But would want to find some way to get the ellipsis after rather than before or else make it more informational (e.g. it could give the number of custom outputs or say "N more" then disappear when all are revealed).

@kinow
Copy link
Member Author

kinow commented Feb 18, 2021

Done, from 40 commits, down to the 14 that I could group together. 5 main commits, the rest are addressing feedback (move + symbol, show extra messages, not total, etc).

Tested locally, everything worked. Ready again for review. 🤓

@kinow
Copy link
Member Author

kinow commented Feb 18, 2021

BTW, tested after syncing all projects to master. Used cylc play --no-detach, but it wouldn't execute the workflow, no matter what. It would just quickly run/play and stop without any error. I didn't want to spend too much time, so I just copied the flow.cylc file elsewhere, nuked the directory, re-created it, and then starting with just 530/flow.cylc it worked 🏅

@hjoliver
Copy link
Member

Wow, well done @kinow, that sounds like it was a gnarly one to figure out! (I presume you've put enough comments in to explain how that works for mere mortals?)

@kinow
Copy link
Member Author

kinow commented Feb 18, 2021

Wow, well done @kinow, that sounds like it was a gnarly one to figure out!

🙌

(I presume you've put enough comments in to explain how that works for mere mortals?)

Bruno opens the IDE to write a comment about the :key used… ops! 😅

@oliver-sanders
Copy link
Member

Used cylc play --no-detach, but it wouldn't execute the workflow, no matter what.

The cylc play command restarts flows by default so if the flow has run to completion it will just shut down straight away.

The simple way to force a flow to run again without re-installing it is to:

rm ~/cylc-run/<flow>/.service/db

We will make this a little easier with cylc clean in the near future, something like:

# remove the db
$ cylc clean <flow> --rm .service/db

# move the db to <timestamp>-db
$ cylc clean <flow> --mv .service/db

@hjoliver
Copy link
Member

The simple way to force a flow to run again without re-installing it is to:

I missed @kinow 's comment on cylc play, I was responding to his previous comment, we must have been writing at the same time. Anyhow, I just re-install each time now: cylc install -> cylc play foo/run1 ... cylc install -> cylc play foo/run2 ... etc.

Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still pretty hazy on the Vue.js but LGTM, and tests as working really well now. 👍

@oliver-sanders
Copy link
Member

oliver-sanders commented Feb 24, 2021

Here's my facetious output generating workflow with needlessly long message strings:

#!Jinja2

{% from "random" import getrandbits %}
{% from "random" import random %}

{% set outputs = [] %}
{% set num_outputs = 100 %}
{% set max_label_len = 30 %}
{% set max_msg_len = 500 %}

{% for _ in range(0, num_outputs) %}
    {% set label = getrandbits((random() * max_label_len) | int + 1) %}
    {% set msg = getrandbits((random() * max_msg_len) | int + 1) %}
    {% do outputs.append((label, msg)) %}
{% endfor %}

[scheduling]
    [[graph]]
        R1 = foo

[runtime]
    [[foo]]
        script = """
{% for label, message in outputs %}
            # {{ label }}
            cylc message -- {{ message }}
            sleep 1
{% endfor %}
        """
        [[[outputs]]]
{% for label, message in outputs %}
            {{ label }} = {{ message }}
{% endfor %}

And here's how it looks in the UI:

outputs2

👍

Thanks for the scrolling info box.

@kinow
Copy link
Member Author

kinow commented Feb 24, 2021

Will fix conflicts in a bit. I think you started the workflow with --hold, then used the aotf mutation to start it?

A good way to test the UI, especially with workflows that have just 1 cycle.

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.

display message triggers
5 participants