Skip to content

Conversation

samvrlewis
Copy link
Contributor

Allows observations to be filtered, as in the old console. Additionally, fixes duplicate obs entries and removes old obs entries when they're not part of the current epoch.

Screenshot from 2021-10-26 16-32-27

There's some remaining work to go on this tab to make it work perfectly, but I think I'll move it off into additional stories unless anyone disagrees.

The first is that if the table columns lose their width if all elements are filtered out (or if all OBS disappear). This I think was a pre-existing issue but never showed because we weren't previously removing old OBS:

obstableresize

The second is that the table is completely missing if there's no obs messages at all (like when the antenna is disconnected). This was also a preexisting issue:

Screenshot from 2021-10-26 16-38-27

@samvrlewis samvrlewis requested a review from a team October 26, 2021 05:40
@john-michaelburke
Copy link
Collaborator

john-michaelburke commented Oct 26, 2021

The second is that the table is completely missing if there's no obs messages at all (like when the antenna is disconnected). This was also a preexisting issue:

We are discerning the width of each column based on the max length of a text in each row / header assuming the font family and pointsize. What I did in another was to fill in each row with an empty string when the app is first run, albeit this was fairly easy with our model set in QML I'm not sure how you'd do this with the python table model. I think behavior that may be ideal would be to just set this fake data by default if no data is received. I have also noticed if I connect to a device that does not get any remote messages but a previously connected device did have remote messages it will display the old ones from the previous device which could be misleading this could also be alleviated by this fake data.

Additionally, I wonder if we could always show the checkboxes for the case of no data and using a fake row not having the checkboxes makes it seem like the centering is off for the remote table.
Screen Shot 2021-10-26 at 10 02 20 AM

In the case of having a fake data row (I'm assuming it would look similar to the remote table in that photo). I'm not a fan of the whitespace at the edge. @silverjam What are your thoughts on the column resizing approach in the observation table in contrast to the "take a fraction of all available width". What we use currently more closely matches the old console and allows making the table expand beyond the width of the app (try dragging the far right column out). This approach may not be ideal when resizing the app as the column widths do not scale with the resizing of the window. I think these UI changes could be meshed with what we currently have though. All things considered probably worth pushing out these feature changes to a separate PR.

@silverjam
Copy link
Contributor

silverjam commented Oct 26, 2021

The second is that the table is completely missing if there's no obs messages at all (like when the antenna is disconnected). This was also a preexisting issue:

We are discerning the width of each column based on the max length of a text in each row / header assuming the font family and pointsize. What I did in another was to fill in each row with an empty string when the app is first run, albeit this was fairly easy with our model set in QML I'm not sure how you'd do this with the python table model. I think behavior that may be ideal would be to just set this fake data by default if no data is received. I have also noticed if I connect to a device that does not get any remote messages but a previously connected device did have remote messages it will display the old ones from the previous device which could be misleading this could also be alleviated by this fake data.

I agree, we should do this so that the tables sized correctly by default.

Additionally, I wonder if we could always show the checkboxes for the case of no data and using a fake row not having the checkboxes makes it seem like the centering is off for the remote table. [...]

I would like to propose not using checkboxes at all... I think it produces a lot of wasted space in the UI. Instead I think we can use a list box with embedded checkboxes to configure a filter for each type of messages.

In the case of having a fake data row (I'm assuming it would look similar to the remote table in that photo). I'm not a fan of the whitespace at the edge. @silverjam What are your thoughts on the column resizing approach in the observation table in contrast to the "take a fraction of all available width". What we use currently more closely matches the old console and allows making the table expand beyond the width of the app (try dragging the far right column out). This approach may not be ideal when resizing the app as the column widths do not scale with the resizing of the window. I think these UI changes could be meshed with what we currently have though. All things considered probably worth pushing out these feature changes to a separate PR.

I don't think the resize model in the obs table is appropriate for our data, if the data we displayed was static and if it was important to see exactly what the values where then I think this resize behavior makes sense. Our data is very ephemeral and not very wide, so we can lean on that to simplify the behavior we need for column sizing... so, long story short: I think we should standardize on the "resize to fill available space" behavior that the other tabs have.

Co-authored-by: John-Michael Burke <[email protected]>
@samvrlewis
Copy link
Contributor Author

samvrlewis commented Oct 26, 2021

The second is that the table is completely missing if there's no obs messages at all (like when the antenna is disconnected). This was also a preexisting issue:

We are discerning the width of each column based on the max length of a text in each row / header assuming the font family and pointsize. What I did in another was to fill in each row with an empty string when the app is first run, albeit this was fairly easy with our model set in QML I'm not sure how you'd do this with the python table model. I think behavior that may be ideal would be to just set this fake data by default if no data is received. I have also noticed if I connect to a device that does not get any remote messages but a previously connected device did have remote messages it will display the old ones from the previous device which could be misleading this could also be alleviated by this fake data.

I guess we might be able to do this in Python by detecting the case when there aren't any rows and then adding this fake row? I'll give it a try.

Additionally, I wonder if we could always show the checkboxes for the case of no data and using a fake row not having the checkboxes makes it seem like the centering is off for the remote table.

At the moment the checkboxes are only shown for the active codes, so this probably wouldn't work. I'm not sure if we want to show all the codes all the time, but could see an argument for that.

I would like to propose not using checkboxes at all... I think it produces a lot of wasted space in the UI. Instead I think we can use a list box with embedded checkboxes to configure a filter for each type of messages.

Do you mean something like this? (Styling can probably be improved). Ignore that I'm showing all of the codes in the first column - this was just to enable there to be enough data to actually scroll.

listview

Or were you thinking that each type of signal would have all of the possible types shown in the listview, even if they weren't currently present? I guess this might be nice as you can pre-select what you want and don't want to see, but at the same time maybe it's an overload of information. I guess it depends on what this tab is used for.. I don't have a clear understanding of that but I could whip up a few prototypes to see what people in the apps team think.

@keithel-qt
Copy link
Contributor

Regarding filling in dummy data to get the column widths set appropriately, I think that would be fine. Just preload the data structure were using to store row data in the abstract table model. We copy over the values from the capnproto decoded structures so it's not refreshed at the same rate as the data coming in, but instead the rate is determined by the timer in QML.

@keithel-qt
Copy link
Contributor

However, if the resize logic is going to move to J-Ms implementation, this might not be needed.

@silverjam
Copy link
Contributor

Do you mean something like this? (Styling can probably be improved). Ignore that I'm showing all of the codes in the first column - this was just to enable there to be enough data to actually scroll.

As discussed in stand-up, I meant a combobox/menubox with checkable/toggable items but we'll defer this work to a future story. I like the scrolling here though, I'd be in favor of keeping that.

Does the counting in the front end, to allow the codes property to be
used and trigger updates
@samvrlewis
Copy link
Contributor Author

Well that was much harder than I thought it would be! After much experimentation, I've hacked something together that will now show the tables even if there aren't any entries, and make clear that they're blank.

The bad news is that I had to remove the column width altering part of the table which might make it a non-starter. :( I just wasn't able to get the table header to show while also allowing the columns to be changed. You can still make the whole app bigger to make the table bigger, though.

I think the proper way of doing this might be to create our own custom table type though I'm not sure how hard that might be.

Screenshot from 2021-10-27 21-25-38
Screenshot from 2021-10-27 21-25-14

@samvrlewis samvrlewis marked this pull request as ready for review October 27, 2021 11:07
@keithel-qt
Copy link
Contributor

The bad news is that I had to remove the column width altering part of the table which might make it a non-starter. :( I just wasn't able to get the table header to show while also allowing the columns to be changed. You can still make the whole app bigger to make the table bigger, though.

Yeah, I think that column resizing is desired, however we can redo the implementation to be like how J-M does. He has a helper method that basically resizes the next column over to the right as you resize.

@john-michaelburke
Copy link
Collaborator

Threw some changes together here: #188

You may be able to reduce some of the code in the label if you merge tip of tree. It should then inherit some font attributes for Label such as pointSize and family then can remove from here.

@samvrlewis
Copy link
Contributor Author

Final iteration (hopefully 😃).

Have used @john-michaelburke 's recommendations and removed the ability to have the table header on empty tables due to difficulties with getting it play nicely with everything else. I think this looks OK with the group box around it, though eventually maybe it would be nice to have another go at having something that better shows the empty table.

Screenshot from 2021-10-28 09-33-51

@samvrlewis samvrlewis requested a review from a team October 27, 2021 22:37
@silverjam
Copy link
Contributor

I think we need to trigger a relayout of the table columns when the app resizes, I'm see some odd behavior in the table when resizing the app:

console-obs-resize

@samvrlewis
Copy link
Contributor Author

samvrlewis commented Oct 28, 2021

I think we need to trigger a relayout of the table columns when the app resizes,

I think this should be happening in the onWidthChanged callback but it looks like there's some weirdness happening. I added a print to the onWidthChanged callback in both the ObservationTable and also the ThreadStateTable (which the obs table is now nearly identical to) and resized the app a bit. The obs table callbacks seem to take longer to arrive (which I guess makes the animation a bit jerky?) and they also don't seem to fire at all when the width is reduced.

widthcallback

Maybe @keithel-qt might know what's going on here?

@silverjam
Copy link
Contributor

silverjam commented Oct 28, 2021

I think we need to trigger a relayout of the table columns when the app resizes,

I think this should be happening in the onWidthChanged callback but it looks like there's some weirdness happening. I added a print to the onWidthChanged callback in both the ObservationTable and also the ThreadStateTable (which the obs table is now nearly identical to) and resized the app a bit. The obs table callbacks seem to take longer to arrive (which I guess makes the animation a bit jerky?) and they also don't seem to fire at all when the width is reduced.

[..]

Maybe @keithel-qt might know what's going on here?

Testing on macOS, the resize notifications seem to happen when growing the app width but I'm also not seeing them happen when shrinking the app. Also not seeing the layout update happen when maximizing the app.

@john-michaelburke
Copy link
Collaborator

john-michaelburke commented Oct 28, 2021

So I am seeing decent resizing when I throw the onWidthChanged block on the ColumnLayout (and removing from tableview):

onWidthChanged: {
    innerTable.forceLayout();
}

I think there may be a small bug still which is parent.width does not equate to the actual available width, for example if you make the app larger then reduce it to the minimum, a portion of the far right column bleeds out of the app. Once you resize any column it snaps back in place. Could be margins/padding/spacing any number of things I think. Actually looks like if you just additionally add an onHeightChanged it resolves the issue. Just add directly under the onWidthChanged.

onHeightChanged: {
    innerTable.forceLayout();
}

@samvrlewis samvrlewis merged commit b313e70 into main Oct 28, 2021
@samvrlewis samvrlewis deleted the slewis/cpp-365 branch October 28, 2021 23:55
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.

4 participants