Configurable Ping #20
Conversation
CLI flags/help new Settings methods: - Update(): overwrite *only* specified fields to claws.json - Clone(): get a goroutine-safe copy for read ops - PushAction(): moved from state.go - LoadSettings(): create new Settings struct from claws.json - ParseFlags(): parse/merge CLI flags into struct RWMutex-ing of access to (global) Settings struct contents state.Settings to instance instead of pointer WebSocket: - clear/close props individually instead of using `closed` bool - added websocket ping message support
- persist time.Ticker & use Stop/Reset instead of nil-ing pointer - added to README & `--help` - UIMode type for tighter type-checking
- parameterized rather than global for State{} data
- re-working of websocket concurrency
- mutex UI writes (since they're called from multiple goroutines)
- websocket message type handling (text vs binary vs control)
- hex dump formatting of binary messages
- indentation of ordinary user/server messages in UI
- adjusted attemptJSONFormatting() to use []byte instead of string (for
websocket.BinaryMessage)
|
Later commits were to address potential race conditions (esp in websocket.go). Given the nature of the program, it was unlikely that they would ever be triggered, but even so. If these are acceptable, I will probably add cmdline opts for specifying HTTP headers to send on websocket connect. |
There was a problem hiding this comment.
Thanks for your contribution! Sorry for the delay in answering, I hadn't my GitHub email notifications set up correctly. There are a few changes, a bunch of them style-wise. There are also general observations I'd like for you to address:
- Could you please change the comments and any/all variables/constants I've missed which are in caps?
- From my point of view, adding a newline after the signature of the function does more harm than good. So if you could please remove that
Appreciate doing the work of removing the reliance on global variables, though maybe that could have been done on a separate PR, for next time ;)
Let's also leave HTTP headers for the next PR, so we get this one out of the door first. Promise I'll be a bit more responsive if emails come through correctly!
Cheers 🥂
| func (s *State) printToOut( | ||
| str string, | ||
| bIndent bool, | ||
| f func(io.Writer, ...interface{}) (int, error), | ||
| ) { |
There was a problem hiding this comment.
I saw that you changed this to have better formatting for multi-line output. I like that, however I do think that it is useful only in the following circumstances:
- We need to print a multiline string (including a pretty-printed JSON object)
- There is a timestamp.
I think it looks weird in other occasions. For instance, timestamp + singleline message just seems unnecessary:
2022-08-24 13:28:25
{"type": "ping"}
2022-08-24 13:28:25
{"type":"pong"}
It doesn't even help distinguishing from real communication messages and logs from the program, as the colors on the output serve that purpose.
Then as well without a timestamp, we're indenting with no "upper layer" so to speak? So we're just padding with two spaces with no real reason.
I think it might just be worth to remove bIndent altogether and determine whether to indent based on strings.IndexByte(str, '\n') >= 0 && len(oSet.Timestamp) > 0
There was a problem hiding this comment.
of all the things i've tried, i think never indenting the sends (same line as ts), & always indenting the receives (line below ts) is clearest. the colors are nice, but not universally distinguishable.
for the sake of "loose coupling", printToOut() should probably be rather dumb, only do what it is explicitly told, & leave the specifics up to its callers.
There was a problem hiding this comment.
and here's what it looks like with timestamps:
|
I can take care of most of these this evening.
Will need to ponder the formatting in printToOut() though, & see if I
can come up with something mutually pleasing. Have you tried the binary
messages feature? There's always room for improvement, but the indent
felt right for binary mode, and not everyone can see colors.
Thx.
JS
|
|
Fair enough re: accessibility. Not thinking enough about accessibility is probably one of my deficiencies. I haven't had a chance to build & test yet, will do that on second round of review |
main.go: error rename + lowercase comments settings.go: var renames & help to stderr websocket.go: var renames, PingMessage payload fmt state.go: var renames mode.go: `modeChars` from map to array editor.go: - autosize "help" view to message size - preliminary status bar impl (commented out) - moved modeBox() closer to where it is used - `enterActions` from map to array - removed IsOpen check on :c (now websocket auto-closes previous conn)
|
Applied the requested adjustments, aside from those regarding |
|
not a github person. apparently i didn't want a "close"... |
- homogenized printToOut() wrappers - switched to one-liner for indentation - never indent request, always indent response - split most of Clone() into SettingsBase
|
just pushed the formatting adjustments |
|
Hi Jason, sorry for the very long tie period between the last activity on this last pull request and now. I didn't have immediate time to take care of this and I knew reviewing this batch of changes would have taken me a few hours, so I procrastinated this for a while longer than I'd like to admit. To you I offer my sincere apologies as I should have at least given an update on this PR, but in any case the PR is now merged into master. I had some changes I wanted to see before this PR was merged; since such a long time has passed I've taken care of these myself. Including nitpicking work on code style, and some matters of personal preference that you had changed in the codebase. Most notably I found a solution that could be visually pleasing to my eyes while taking into consideration your suggestion about making the software accessible and usable even without the aid of colors. I'm not a fan of distinguishing this using indentation or not, and I found it visually asymmetric that the outbound messages were not indented while the inbound were not. Ideally, a cool idea would be to have an optional "sidebar" on the message viewer which can act both as a line counter and a text indicator of which is in an inbound and which is an outbound message. Something like: (where the dots are used to indicate lines without breaks that overflow). However this approach is quite complex (because of handling screen resizes with wrapped lines, making it look nice, etc.) and I'm not even completely sure I can do it the way I want it to with gocui (which I'd need to get my hands dirty on again). So for now I went for a simpler solution, which though should enable visual distinctions effectively: the default format for timestamp now has a Again, apologies this has taken so long. This was mostly because I knew properly addressing and reviewing your code would have taken a significant amount of time. I appreciate if you have other suggestions or contributions; however I'd ask you to keep them more concentrated and focused next time, so that also the diffs as a result are clear, as for this one I've had to keep quite a few tabs of my text editor and GitHub open at the same time, trying to understand where all the code was reshuffled. Thanks! |
|
No problem. My use case had diverged so much from the adjustments I made to Claws that I rolled my own as a simple STDIN/OUT program instead of a TUI:
https://github.com/BourgeoisBear/wscli
But thank you for claws.
JS
… On Jan 29, 2023, at 6:32 PM, Morgan ***@***.***> wrote:
Hi Jason, sorry for the very long tie period between the last activity on this last pull request and now. I didn't have immediate time to take care of this and I knew reviewing this batch of changes would have taken me a few hours, so I procrastinated this for a while longer than I'd like to admit. To you I offer my sincere apologies as I should have at least given an update on this PR, but in any case the PR is now merged into master.
I had some changes I wanted to see before this PR was merged; since such a long time has passed I've taken care of these myself. Including nitpicking work on code style, and some matters of personal preference that you had changed in the codebase. Most notably I found a solution that could be visually pleasing to my eyes while taking into consideration your suggestion about making the software accessible and usable even without the aid of colors. I'm not a fan of distinguishing this using indentation or not, and I found it visually asymmetric that the outbound messages were not indented while the inbound were not.
Ideally, a cool idea would be to have an optional "sidebar" on the message viewer which can act both as a line counter and a text indicator of which is in an inbound and which is an outbound message. Something like:
=> |
2 |
. |
. |
3 |
<= |
=> |
<= |
(where the dots are used to indicate lines without breaks that overflow).
However this approach is quite complex (because of handling screen resizes with wrapped lines, making it look nice, etc.) and I'm not even completely sure I can do it the way I want it to with gocui (which I'd need to get my hands dirty on again). So for now I went for a simpler solution, which though should enable visual distinctions effectively: the default format for timestamp now has a => prepended to it; this is changed to <=, == and !! as needed, based on the type of message. And both inbound and outbound messages are now indented; but only if timestamps are enabled, which thus becomes a kind of way also to use accessibility features.
Again, apologies this has taken so long. This was mostly because I knew properly addressing and reviewing your code would have taken a significant amount of time. I appreciate if you have other suggestions or contributions; however I'd ask you to keep them more concentrated and focused next time, so that also the diffs as a result are clear, as for this one I've had to keep quite a few tabs of my text editor and GitHub open at the same time, trying to understand where all the code was reshuffled.
Thanks!
Morgan
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you modified the open/close state.
|
Lots of surgery on settings to get there. Let me know how it looks.
Thx.
JS