-
-
Notifications
You must be signed in to change notification settings - Fork 128
Modified the way the command string is constructed for calling the text-... #26
Conversation
…xt-based browsers (w3m, lynx, etc.).
Whilst I agree that having a hard coded -cols 70 is a bad idea. But this change will break any scripts that depend on that fact. What about making the default w3m command include the -cols 70, then it can still be overridden from a2x.conf but the default doesn't change. Also removing the quotes may break some systems, what did you test on? In particular I'm suspicious that it will break on Windows. Edit: of course it will also break on filenames with spaces in them, which is unacceptable. |
the -cols 70 setting used not be there not so long ago IIRC (a few releases back) so I don't think this is a 'sacrosanct' default. I also could not easily imagine a (post-processing?) script depending on this exact figure. for an important default of course your proposal would be better but would imply to analyze the command string (i.e. programatically remove `-cols 70' if that arg is detected to be present in the value of W3M -- well, for that I might need to first learn python as well a bit) . and it also would keep the asymmetry between LYNX (uses LYNX default width) and W3M (does use a different default than that of W3M) which I think is not really good: if not specified explictly, I feel the browser-interal text-width default should always be used. so I would opt for removing the cols 70 int the W3M block (which restores previous behaviour, I believe, see above). what do you think? testing: this was under MacOS and linux. no idea about windows but I don' t think there would be a new problem there. but regarding blanks in filenames, you are right of course (would not have made this mistake in a shell script, also I never ever use file names with blanks myself ...). I have now restored the quoting of both file names (2. and 3. `%s'). it now works with blanks-containing file names as well (tested). sorry for being to quick with the intial PR. I've just pushed the fix and hope that the PR is aware of this added change (or need I to renew the PR?). |
I've just seen that the PR indeed recognizes both commits. so this now should work. the only question, then, is whether to keep the '-col 70' (non-)default width as 'a2x' default in the W3M block? as already stated I'd vote for not doing this. |
and regarding the 'windows' question: I have no good idea how the command line works there in detail, but would think the change is not introducing a new problem: in both cases (old and new) a single string argument to the 'shell' function call is constructed which was (and thus should be) correctly parsed by the respective command interpreter (unix/dos shell). |
Yes, but the original command was in quotes, and sadly common places for programs on Windows is "Program Files" (or something similar, I'm not a windows expert). And on Linux or Windows, any place LYNX or W3M are installed that is configured in a2x.conf is precluded from having spaces in the path since its now unquoted. I agree with the idea of what you are trying to do, its just we need to come to a suitable implementation. Suggestion to try: Add a LYNX_OPTS and W3M_OPTS that the user can override in a2x.conf and include them in the command string like: W3M_OPTS default value would be On the -cols 70, well somebody added it, so something must depend on it :) Note that I am not suggesting parsing any settings the user provides in a2x.conf to see if they put -cols, if they put any value in a2x.conf it will completely override the default. Trying to combine two sets of arguments is a recipe for disaster, never do it :) |
OK, I see your point regarding the possibility of blanks in the programs pathname. It would then to have been done like you suggest. I will have a look ASAP (no time right now). |
Well, the -cols 70 has been in a2x.py since its inception in 2009, and it was in the predecessor a2x shell script at that time, but I was too lazy to trace that back any further :) |
…cted text-based browser.
I've adjusted the fix now along the line you've proposed. I only left out the `-dump' since this flag needs always to be there (and has the same name with all text-based browsers I know (w3m, lynx, links, elinks). does this look good now? |
Looks ok, but you just need to document it :) My personal preference would be to not make |
…ocument the changes (also fixed a few typos).
I agree. |
Modified the way the command string is constructed for calling the text-browsers
Sorry for the merge delay. |
no problem. happy to see it accepted. |
...based browsers (w3m, lynx, etc.).
this fixes the previously existing limitation that it was not possible to pass any arguments to the selected text-based browser by defining an entry in `a2x.conf' such as
LYNX = "elinks -dump-width 80"
moreover, the old call for `w3m' (the default) did explicitly set a (rather small) width. I removed this setting so that behaviour is more consistent (specify width always explicitly, if desired, for the selected browser (defined via W3M or LYNX) or accept the default width used by that browser.