review of man page #87

asdf · 2019-11-13T03:26:30Z

asdf commented

2019-11-13 03:26:30 +00:00

Reviewed for #64

asdf added this to the 2.0.0 milestone 2019-11-13 03:26:30 +00:00

asdf self-assigned this 2019-11-13 03:26:30 +00:00

sloum was assigned by asdf

2019-11-13 03:26:30 +00:00

asdf added the

documentation

label 2019-11-13 03:26:30 +00:00

sloum approved these changes 2019-11-13 04:25:44 +00:00

sloum left a comment

I left one comment. I think it makes sense and is correct. If you find that it is not feel free to merge in as is. Otherwise, I approve pending a change to the commented area (and feel free to merge in after that).

bombadillo.1 Outdated

						
				@ -210,3 +212,3 @@

				.B

				lynxmode

				Will use lynx as a rendering engine for http/https requests if lynx is installed and fIopenhttpfP is set to fItruefP. Valid values are fItruefP and fIfalsefP.

				Will use lynx as a rendering engine for http/https requests if lynx is installed and both fIopenhttpfP and fIterminalonlyfP are set to fItruefP. Valid values are fItruefP and fIfalsefP.

sloum commented

2019-11-13 04:23:13 +00:00

I do not believe this update to be correct. If lynxmode and 'openhttpare both set to true then lynx will be used regardless of whetherterminalonlyis set to true or not. This enables desktop users, like myself, that want to render in the client to do so more easily. To access open in browser featureslynxmodemust be false andopenhttpmust be true, andterminalonly` must be false.

I do not believe this update to be correct. If `lynxmode` and 'openhttp` are both set to true then lynx will be used regardless of whether `terminalonly` is set to true or not. This enables desktop users, like myself, that want to render in the client to do so more easily. To access open in browser features `lynxmode` must be false and `openhttp` must be true, and `terminalonly` must be false.

asdf commented

2019-11-14 01:00:21 +00:00

I've amended the documentation based on your feedback. I'd appreciate it if you could double-check it, and merge if OK. I found it a bit tricky to document the different states of terminalonly and lynxmode, but I think this now is both correct and conveys the use cases.

I've amended the documentation based on your feedback. I'd appreciate it if you could double-check it, and merge if OK. I found it a bit tricky to document the different states of `terminalonly` and `lynxmode`, but I think this now is both correct and conveys the use cases.

sloum commented

2019-11-14 02:08:16 +00:00

I am starting to agree with what you wrote elsewhere: three settings is a confusing situation. Should we ditch lynxmode as an option and just have terminalonly for determining how to open http links?

If a user tries to open an http link (assuming openhttp is true) with terminalonly as true and lynx is not installed, we can set it up to give an error indicating that fact.

That would leave things with this grid:

openhttp	terminalonly	lynx avail.	result
true	false	any	Tries to open in web browser
true	true	true	Uses lynx for rendering
true	true	false	Error message about lynx not being present
false	any	any	Error message about web links not enabled

That leaves me wondering if terminalonly is still an accurate name. gui, webgui, guimode, terminalmode, terminal.

Or there is what you had recommended, I think...correct me if I'm wrong: That we switch it from a boolean and just have a config setting of webmode and have the following available none, gui, lynx, raw (I could likely make a quick and dirty raw mode that just retrieves the html and does no rendering, though that mode could get added after 2.0.0 if it were deemed useful).

I know this is a lot to think through. I appreciate you going through the manpage and making the updates and I'm sorry for being wishy-washy on this subject.

I am starting to agree with what you wrote elsewhere: three settings is a confusing situation. Should we ditch `lynxmode` as an option and just have `terminalonly` for determining how to open http links? If a user tries to open an http link (assuming `openhttp` is true) with `terminalonly` as true and lynx is not installed, we can set it up to give an error indicating that fact. That would leave things with this grid: | openhttp | terminalonly | lynx avail. | result | |----------|--------------|-------------|--------------------------------------------| | true | false | any | Tries to open in web browser | | true | true | true | Uses lynx for rendering | | true | true | false | Error message about lynx not being present | | false | any | any | Error message about web links not enabled | That leaves me wondering if `terminalonly` is still an accurate name. `gui`, `webgui`, `guimode`, `terminalmode`, `terminal`. Or there is what you had recommended, I think...correct me if I'm wrong: That we switch it from a boolean and just have a config setting of `webmode` and have the following available `none`, `gui`, `lynx`, `raw` (I could likely make a quick and dirty raw mode that just retrieves the html and does no rendering, though that mode could get added after 2.0.0 if it were deemed useful). I know this is a lot to think through. I appreciate you going through the manpage and making the updates and I'm sorry for being wishy-washy on this subject.

asdf commented

2019-11-14 02:29:44 +00:00

That's all good, no worries from my part at all. It's fun to work these things out.

I was writing this documentation with the idea in mind that we would consider changes to the functionality beyond version 2.0.0. If we did that:

the current state might cause some confusion
when we eventually make the change, it's possibly another big impact to any person's .bombadillo.ini, like the 2.0.0 release

Perhaps we should rename this to "review of man page", merge it in as-is, then discuss the next steps in #86.

That's all good, no worries from my part at all. It's fun to work these things out. I was writing this documentation with the idea in mind that we would consider changes to the functionality beyond version 2.0.0. If we did that: - the current state might cause some confusion - when we eventually make the change, it's possibly another big impact to any person's `.bombadillo.ini`, like the 2.0.0 release Perhaps we should rename this to "review of man page", merge it in as-is, then discuss the next steps in #86.

sloum commented

2019-11-14 02:41:47 +00:00

Awesome. I agree, lets rename this PR and merge it in (I'll do that now) and resume conversation on #86 . I do think it would be good to sort it before release of 2.0.0 to avoid just what you mention: major ini changes right after people newly adopt and use a system.