-
Notifications
You must be signed in to change notification settings - Fork 2
[CPP-365] [CPP-355] Allow filtering of the OBS tables #182
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
Conversation
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. 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 agree, we should do this so that the tables sized correctly by default.
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.
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]>
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.
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.
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. 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. |
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. |
However, if the resize logic is going to move to J-Ms implementation, this might not be needed. |
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
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. |
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. |
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. |
* Fix obs column headers. * Add back in bottom rect Co-authored-by: Sam Lewis <[email protected]>
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. |
I think this should be happening in the 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. |
So I am seeing decent resizing when I throw the onWidthChanged block on the ColumnLayout (and removing from tableview):
|
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.
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:
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: