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

Add logic to report status message to FreeDV Reporter. #620

Merged
merged 37 commits into from Dec 24, 2023
Merged

Conversation

tmiw
Copy link
Collaborator

@tmiw tmiw commented Dec 10, 2023

Resolves #602 by adding functionality to the FreeDV Reporter window to allow live reporting of a user-defined status message. Example:

Screenshot 2023-12-10 at 2 28 31 PM

@Tyrbiter
Copy link

Message is appearing in the app reporter, but not on the web version. Message column is showing after a js refresh.

@tmiw
Copy link
Collaborator Author

tmiw commented Dec 11, 2023

Message is appearing in the app reporter, but not on the web version. Message column is showing after a js refresh.

Does holding down Shift while pressing F5 on the Web version improve this at all?

@Tyrbiter
Copy link

Message is appearing in the app reporter, but not on the web version. Message column is showing after a js refresh.

Does holding down Shift while pressing F5 on the Web version improve this at all?

Not using Firefox 120.0.1, shift-refresh doesn't, shift-F5 opens a browser recording console window of some sort.

I also tried Chromium, that doesn't display the message text either, although it displays the Message column.

@tmiw
Copy link
Collaborator Author

tmiw commented Dec 11, 2023

Message is appearing in the app reporter, but not on the web version. Message column is showing after a js refresh.

Does holding down Shift while pressing F5 on the Web version improve this at all?

Not using Firefox 120.0.1, shift-refresh doesn't, shift-F5 opens a browser recording console window of some sort.

I also tried Chromium, that doesn't display the message text either, although it displays the Message column.

Weird, seems to work for me?

Screenshot 2023-12-11 at 10 49 36 AM

I did find another issue, though--reloading the page clears the message (but editing it will bring it back). This is probably something server-side, so I'll look into it tonight.

@Tyrbiter
Copy link

Tyrbiter commented Dec 11, 2023

I saw your message when I looked at the page without reloading it, but nothing from me showed up.

Refreshing it again cleared your message, so clearly there is something that needs fixing.

Editing my message showed it until I refreshed the page, so it's repeatable.

@tmiw
Copy link
Collaborator Author

tmiw commented Dec 12, 2023

I saw your message when I looked at the page without reloading it, but nothing from me showed up.

Refreshing it again cleared your message, so clearly there is something that needs fixing.

Editing my message showed it until I refreshed the page, so it's repeatable.

I think I found out what was causing this. Restarted the server with the required changes, hopefully that works a bit better.

@Tyrbiter
Copy link

Server fix looks good.

Am I right in thinking that the message needs to be edited to be active? It seems that way. A previous message is not shown until I edit it, either in the app or on the web.

@tmiw
Copy link
Collaborator Author

tmiw commented Dec 12, 2023

Server fix looks good.

Am I right in thinking that the message needs to be edited to be active? It seems that way. A previous message is not shown until I edit it, either in the app or on the web.

Do you mean when you restart FreeDV? For me, restarting FreeDV brings back the previous message I entered.

@Tyrbiter
Copy link

Tyrbiter commented Dec 12, 2023

Yes, it does for me now, might have been down to whatever the server change did or maybe some changes with another PR merged in here. I wonder if it would be useful to remind a user of the message somehow, or perhaps have an option to use the existing message on start.

@tmiw
Copy link
Collaborator Author

tmiw commented Dec 12, 2023

Yes, it does for me now, might have been down to whatever the server change did or maybe some changes with another PR merged in here. I wonder if it would be useful to remind a user of the message somehow, or perhaps have an option to use the existing message on start.

I'm not sure we need a separate config option for this. Maybe @barjac can chime in?

@Tyrbiter
Copy link

I was thinking about something to indicate that a message is set when the reporter window is closed.

@tmiw
Copy link
Collaborator Author

tmiw commented Dec 13, 2023

I was thinking about something to indicate that a message is set when the reporter window is closed.

Since the FreeDV Reporter window state persists between executions (i.e. it reopens if it was open when you terminated FreeDV), seeing a message there when it comes back might be enough, right? I'm also presuming that a lot of the use cases for this (i.e. indicating that you're using a WebSDR for receive) won't necessarily make the previously set message invalid on next execution.

Anyway, we'll see what @barjac (and anyone else who happens on this PR) think before proceeding either way.

@barjac
Copy link

barjac commented Dec 15, 2023

Hi,
Thanks :)
It works communication wise, but for me even only three characters are off the screen because the text in the column is centred. The text in the Msg column could be left justified and the column label only needs three characters: 'Msg'.
This was my reasoning for using only 3 characters to keep the width of the reporter table to a minimum for this aspect ratio of screen.
It is of course not an issue for high aspect ratio screens.
Maybe an option for users to select which columns to show in the reporter would help. Most users will not need to always see the 'Locator' column or 'Last update' perhaps.

I think more space saving in all columns needs to be made for it to work well as it stands.

Screenshot_20231215_152637

@barjac
Copy link

barjac commented Dec 15, 2023

I was thinking about something to indicate that a message is set when the reporter window is closed.

Since the FreeDV Reporter window state persists between executions (i.e. it reopens if it was open when you terminated FreeDV), seeing a message there when it comes back might be enough, right? I'm also presuming that a lot of the use cases for this (i.e. indicating that you're using a WebSDR for receive) won't necessarily make the previously set message invalid on next execution.

Anyway, we'll see what @barjac (and anyone else who happens on this PR) think before proceeding either way.

Maybe a 'Clear' or 'X' button to the right of the message entry box is needed, as the intention is that these messages in the main are temporary, so rather than having to insert the cursor and backspace over the last message a single click will remove it?

@barjac
Copy link

barjac commented Dec 15, 2023

Regarding space saving:
The Frequency column title could be 'MHz' (we all know that 'MHz' indicates frequency) and the values in each record could be only numeric. This would save repeating 'MHz' over and over.

Under 'Status' the word 'Receiving' is too long, 'RX' would suffice. Likewise 'Transmitting' could be TX.

'RX Only' could be totally superseded by simply using 'RXO' in the Msg column.

'TX mode' heading label is forcing the column to be too wide for the data. 'Mode' would suffice.

The display of all the data looks rather scrappy due to the centre justification. It would be more readable if it was all left justified. Especially the 'Version' column.

@Tyrbiter
I limited my reported 'Version' git# to 5 chars to be no wider than 'FreeDV x.x.x-devel' however you now exceed that. Maybe 'FDV-x.x.x-devNN' would suffice for devel versions?
I'm not sure yet where the 'FreeDV' comes from but I guess it's possible to patch it.

A lot to discuss but it's a great start and from comments on the air we are all looking forward to seeing this in action.

@Tyrbiter
Copy link

Regarding the version reporting, I wonder if it would be sensible to display a tool tip with the version in future, perhaps when this has all settled down. At present it's useful to see when a station is using an older, potentially buggy version.

@tmiw
Copy link
Collaborator Author

tmiw commented Dec 16, 2023

'RX Only' could be totally superseded by simply using 'RXO' in the Msg column.

"RXO" in the message field won't necessarily be obvious to the new FreeDV user.

'TX mode' heading label is forcing the column to be too wide for the data. 'Mode' would suffice.

Eh...we list modes for both TX and RX. How would one be able to tell which is which if the TX Mode column is shortened as suggested?

The display of all the data looks rather scrappy due to the centre justification. It would be more readable if it was all left justified. Especially the 'Version' column.

Unfortunately I did some experimentation on this and it doesn't look like wxWidgets allows the column names to be center aligned while also having the column data itself be left-aligned. To me, forcing both column names and column data to be left-aligned looks worse, so I'm leaving the alignment as-is for now.

@Tyrbiter I limited my reported 'Version' git# to 5 chars to be no wider than 'FreeDV x.x.x-devel' however you now exceed that. Maybe 'FDV-x.x.x-devNN' would suffice for devel versions? I'm not sure yet where the 'FreeDV' comes from but I guess it's possible to patch it.

IIRC "FreeDV" comes from the CMake configuration somewhere. Probably not a good idea to change this.

Maybe a 'Clear' or 'X' button to the right of the message entry box is needed, as the intention is that these messages in the main are temporary, so rather than having to insert the cursor and backspace over the last message a single click will remove it?

If these messages are temporary, should they not be preserved at all in the configuration, then? I assumed you guys would want them to be but it should be straightforward to remove the save/load code for this if that'll be better.

@Tyrbiter
Copy link

A compromise might be to have a clear button, then allow the last 2 or 3 messages to be selected from a drop-down list? A new message would remove the earliest one from the drop-down.

Of course, we could wait to decide until more people have had the chance to play with it live.

@barjac
Copy link

barjac commented Dec 17, 2023

'RX Only' could be totally superseded by simply using 'RXO' in the Msg column.

"RXO" in the message field won't necessarily be obvious to the new FreeDV user.

How many characters do you envisage finally allowing? If more than 3 then 6 would cope with 'RXonly'.

'TX mode' heading label is forcing the column to be too wide for the data. 'Mode' would suffice.

Eh...we list modes for both TX and RX. How would one be able to tell which is which if the TX Mode column is shortened as suggested?

'RX mode' is to the right of RX call which makes fairly obvious, so 'Mode' in that position with a tooltip would I think be adequate. Space needs to come from somewhere :/

There is existing padding in all columns which could be reduced. I can do it manually in GUI headers. Can this be tightened up in the code?
This alone would possibly be enough gain assuming 3 characters (and 'Msg' as the column label).

The display of all the data looks rather scrappy due to the centre justification. It would be more readable if it was all left justified. Especially the 'Version' column.

Unfortunately I did some experimentation on this and it doesn't look like wxWidgets allows the column names to be center aligned while also having the column data itself be left-aligned. To me, forcing both column names and column data to be left-aligned looks worse, so I'm leaving the alignment as-is for now.

OK, if we stuck to 3 characters then this may not be a problem, but at present the Msg field is so wide that 3 characters centred puts them so far to the right that they are off screen.

I think that the 'Msg' column should be to the right of the current 'Status' column in the centre of the display.

@Tyrbiter I limited my reported 'Version' git# to 5 chars to be no wider than 'FreeDV x.x.x-devel' however you now exceed that. Maybe 'FDV-x.x.x-devNN' would suffice for devel versions? I'm not sure yet where the 'FreeDV' comes from but I guess it's possible to patch it.

IIRC "FreeDV" comes from the CMake configuration somewhere. Probably not a good idea to change this.

Maybe just remove it then for the reporter? 'x.x.x-devNN', x.x.x-123ab or x.x.x.

Maybe a 'Clear' or 'X' button to the right of the message entry box is needed, as the intention is that these messages in the main are temporary, so rather than having to insert the cursor and backspace over the last message a single click will remove it?

If these messages are temporary, should they not be preserved at all in the configuration, then? I assumed you guys would want them to be but it should be straightforward to remove the save/load code for this if that'll be better.

I did say 'in the main' since some should be preserved like WRX (Web sdr receiving), which for someone who has to use remote RX due to noise it will be a permanent message.
Maybe a 'default' message could be in settings, always used at program start but manually overwritable during a session from the GUI.
I think a 'clear' button and saving between sessions would then be optimum.
If nothing was saved at last close down then use the default if there is one.

@barjac
Copy link

barjac commented Dec 17, 2023

A compromise might be to have a clear button, then allow the last 2 or 3 messages to be selected from a drop-down list? A new message would remove the earliest one from the drop-down.

An MRU list was in the original enhancement request, so yes, I would think 10 a more suitable limit.

Of course, we could wait to decide until more people have had the chance to play with it live.

@tmiw
Copy link
Collaborator Author

tmiw commented Dec 19, 2023

I managed to get a bit more space, I think. However, I'm not fully sure there's enough room for both a Clear and an Add button (the latter I would think being required to have a MRU feature). For example:

Screenshot 2023-12-18 at 8 39 57 PM

Additionally, the alternative for Add (simply implying it when one pushes Enter on the soon-to-be combo box) might not be 100% obvious, either. Suggestions would be good here.

@barjac
Copy link

barjac commented Dec 21, 2023

Just checked this new build and I don't see anything wrong with the VK2 on 30m with band filtering selected, the message column shows up and the -- entries are correctly centred under the column headings.

No neither do I if I restart the reporter.
The problem happened when switching the filter to 30m from All with a message showing in the All view.
Restarting the reporter cleared the fault, so maybe some sort of re-set is required when the filter is changed?

EDIT Just tested again on a different system with the same issue.
Some column labels are also being truncated after switching filter.

The issue seems to occur when the required message column width is different between the old and new filter being selected.

@tmiw
Copy link
Collaborator Author

tmiw commented Dec 21, 2023

I'm able to duplicate by selecting the filters in this order: All->160m->80m->60m. The fix I just checked in seems to resolve it for me, but give it a shot.

Why would anyone use Enter in a drop down list? It's a mouse click action that everyone uses.

I should have clarified. Enter (or pushing the Set button) would be for adding new messages. No need to do either of those things if you select an existing item.

I see no need for a Set button or use of Enter in connection with the MRU list.

Agreed. Hopefully the graying out of it (and the Clear button) when there's nothing new to potentially commit to the list will make that clearer.

Maybe if the MRU are stored in an accessible text file then users could add/remove their favourites at will.

~/.freedv (or ~/Library/Preferences/FreeDV Preferences on macOS), specifically this section:

[Reporting/FreeDV]
Enable=1
...
StatusText=M1 Mac Mini
RecentStatusTexts=M1 Mac Mini,012345678901234,012345678901236,M1 Mac Mini2,M1 2

May be a bit more complicated on Windows, though, since the Registry's used for that platform.

I wonder if a right (or middle) click on the MRU list could be made to delete the item under the cursor? Left click to use, right/middle click to delete?

We'd only have access to the selected item, not the item under the mouse cursor (and selection doesn't change until the box closes again). Also, the proposed right-click behavior definitely wouldn't be obvious to users.

Have you had any thoughts about notification of a change? I was thinking that the line that changed text should change background colour until the end of the next update cycle. (Unless it changes to empty).

What color should be used if this were to be implemented? Same as for RX, or a different one?

@barjac
Copy link

barjac commented Dec 21, 2023

What color should be used if this were to be implemented? Same as for RX, or a different one?

User selectable like the others would be consistent.

Edit - Just thinking more about this...
If possible maybe changing only the colour of the Message itself rather than the whole line would draw better attention to it.

@barjac
Copy link

barjac commented Dec 21, 2023

I'm able to duplicate by selecting the filters in this order: All->160m->80m->60m. The fix I just checked in seems to resolve it for me, but give it a shot.

No it's not fixed.
Starting the reporter with 20m filter enabled and me reporting on 60m I see this which is fine:

Screenshot_20231221_203852

If I then check 'Track' then this also OK:

Screenshot_20231221_204405

If I now un-check 'Track' then we hit the problem:

Screenshot_20231221_204746

Moving from a filter with a message to a filter without causes the problem.

######################################
EDIT: After lots more testing I have concluded that its the column label widths that are not updating to match the data - it is also affecting the Version column as can be seen in the last screen shot above.
If I transmit on 60m and cause an update then the label widths update and are correct.
In a filter where I am not shown the label widths only update when someone on the screen transmits.
######################################

O/T
The message entry box will only display 14.5 characters without scrolling here ;)

@tmiw
Copy link
Collaborator Author

tmiw commented Dec 21, 2023

What color should be used if this were to be implemented? Same as for RX, or a different one?

User selectable like the others would be consistent.

Sure, but what about the default?

Edit - Just thinking more about this... If possible maybe changing only the colour of the Message itself rather than the whole line would draw better attention to it.

I'll have to look at how this is being done for the other colors. It's possible we can only do it for the entire row.

@barjac
Copy link

barjac commented Dec 21, 2023

What color should be used if this were to be implemented? Same as for RX, or a different one?

User selectable like the others would be consistent.

Sure, but what about the default?

The palest purple looks good to me but I have forgotten what the defaults are.
I use the palest orange for TX and palest blue for RX.
The very pale colours provide the highest contrast with black text.

@barjac
Copy link

barjac commented Dec 21, 2023

See EDIT in my post with screen shots.

@tmiw
Copy link
Collaborator Author

tmiw commented Dec 22, 2023

Edit - Just thinking more about this... If possible maybe changing only the colour of the Message itself rather than the whole line would draw better attention to it.

Unfortunately it does indeed look like it has to be adjusted on a row by row basis. The latest implements this with a 5 second timeout (TBD) and the purple background as the default.

Moving from a filter with a message to a filter without causes the problem.

Yeah, I think this is a wxWidgets bug, but I think I was able to work around it now with the latest in this PR.

O/T The message entry box will only display 14.5 characters without scrolling here ;)

The latest widens that field a bit more. I don't think I can go further without making the window unusable on 720p displays, though.

@barjac
Copy link

barjac commented Dec 22, 2023

Tested @ #aff64 and it's looking good, not much left to do :)
Three issues:
When changing from a wide message to a narrow one the Msg column header stays at max width.

Screenshot_20231222_203552

After changing message:

Screenshot_20231222_203654

Not sure I agree with 'Don't highlight when we change our own message.' I rather think it would be reassuring if all users see the same. I would like to see confirmation that my new message was highlighted for everyone. I think without this users will think it's not working. It would also make testing easier ;)

The widened message entry box did not need such a big increase, less than half would have been adequate for my '0123456789abcde' test on the low aspect Dell office type screen. See first image.

@tmiw
Copy link
Collaborator Author

tmiw commented Dec 22, 2023

Three issues: When changing from a wide message to a narrow one the Msg column header stays at max width.

I tried to force that column to always auto-resize on any updates to it but that doesn't seem to be working for some reason. It's weird because I'm pretty sure other columns are able to auto-resize on changes without any issues.

Maybe we can leave it for now and revisit if it ends up becoming a serious issue.

Not sure I agree with 'Don't highlight when we change our own message.'

Reverted.

The widened message entry box did not need such a big increase, less than half would have been adequate for my '0123456789abcde' test on the low aspect Dell office type screen. See first image.

I shrunk it to 180px but let me know if it needs a tiny bit more.

@barjac
Copy link

barjac commented Dec 23, 2023

Three issues: When changing from a wide message to a narrow one the Msg column header stays at max width.

I tried to force that column to always auto-resize on any updates to it but that doesn't seem to be working for some reason. It's weird because I'm pretty sure other columns are able to auto-resize on changes without any issues.

No they don't.
'Version' is the same. If I filter to where users are all using 1.9.x release then switch to that band and start the modem, then the column widens to accomodate my 'FreeDV 1.9.6-0b234' but it fails to revert back when I stop the modem.

Also even if the filter is set to not include me I see a purple row (presumably me) flash up briefly on program start (full FreeDV not just reporter) and the Msg column is set wide for my message even though it's not on the screen.

Maybe we can leave it for now and revisit if it ends up becoming a serious issue.

OK, it would be fine for a 1.9.6 release but with a long Msg and a wide Version the low aspect screen is not far from the limit.

Not sure I agree with 'Don't highlight when we change our own message.'

Reverted.

Good that is better :)

The widened message entry box did not need such a big increase, less than half would have been adequate for my '0123456789abcde' test on the low aspect Dell office type screen. See first image.

I shrunk it to 180px but let me know if it needs a tiny bit more.

Hmm... this is my fault for not finding the widest characters and considering that it's not using a fixed width font.
Only 10 'M's fit the entry box now.
15 'M's cause the Last Update column to be truncated, however this is a corner case and I think that messages that use mainly lower case will not cause a problem.

@tmiw
Copy link
Collaborator Author

tmiw commented Dec 24, 2023

No they don't. 'Version' is the same. If I filter to where users are all using 1.9.x release then switch to that band and start the modem, then the column widens to accomodate my 'FreeDV 1.9.6-0b234' but it fails to revert back when I stop the modem.

For me, it was changing column widths when I was switching band filters. Examples:

Screenshot 2023-12-23 at 10 03 07 PM Screenshot 2023-12-23 at 10 03 15 PM

I did add some additional logic to force resize all columns when rows are removed from the list, though. This should apply both to the filter change case as well as simple disconnects/modem stops.

Also even if the filter is set to not include me I see a purple row (presumably me) flash up briefly on program start (full FreeDV not just reporter) and the Msg column is set wide for my message even though it's not on the screen.

Should be fixed as of the latest, too.

Hmm... this is my fault for not finding the widest characters and considering that it's not using a fixed width font. Only 10 'M's fit the entry box now. 15 'M's cause the Last Update column to be truncated, however this is a corner case and I think that messages that use mainly lower case will not cause a problem.

👍

@barjac
Copy link

barjac commented Dec 24, 2023

All seems fine now :)
One minor improvement may be to make the most recent MRU items appear at the bottom of the list since this list will generally display above the entry box which (for most users) is at the bottom of the screen.
This will put the most commonly used items in easy reach.

@tmiw
Copy link
Collaborator Author

tmiw commented Dec 24, 2023

All seems fine now :) One minor improvement may be to make the most recent MRU items appear at the bottom of the list since this list will generally display above the entry box which (for most users) is at the bottom of the screen. This will put the most commonly used items in easy reach.

👍

I'll leave the drop-down for now and revisit if it ends up that this is preferred instead.

@tmiw tmiw merged commit 6d2f30b into master Dec 24, 2023
2 checks passed
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.

Add new 'Msg' column to reporter.
3 participants