Skip to content
This repository was archived by the owner on Oct 17, 2021. It is now read-only.

Modified the way the command string is constructed for calling the text-... #26

Merged
merged 4 commits into from
May 15, 2014

Conversation

jghub
Copy link
Contributor

@jghub jghub commented May 5, 2014

...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.

@elextr
Copy link
Contributor

elextr commented May 5, 2014

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.

@jghub
Copy link
Contributor Author

jghub commented May 5, 2014

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?).

@jghub
Copy link
Contributor Author

jghub commented May 5, 2014

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.

@jghub
Copy link
Contributor Author

jghub commented May 5, 2014

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).

@elextr
Copy link
Contributor

elextr commented May 5, 2014

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: '"%s" %s "%s">"%s"'% (W3M, W3M_OPTS, html_file, text_file)

W3M_OPTS default value would be "-cols 70 -dump -T text/html -no-graph" so all options are tailorable.

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 :)

@jghub
Copy link
Contributor Author

jghub commented May 5, 2014

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).
regarding the `-cols 70' thing: maybe that person (depending on this "default") could please stand up ;-). but completely overriding instead of "argument merging" is of course fine

@elextr
Copy link
Contributor

elextr commented May 6, 2014

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 :)

@jghub
Copy link
Contributor Author

jghub commented May 7, 2014

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?

@elextr
Copy link
Contributor

elextr commented May 8, 2014

Looks ok, but you just need to document it :)

My personal preference would be to not make -dump fixed since then the user knows that if they override the options they override all of them, rather than all but -dump, lets see how hard it is to explain when you document it :)

…ocument the changes (also fixed a few typos).
@jghub
Copy link
Contributor Author

jghub commented May 8, 2014

I agree. -dump is now included in the definition of the new variables. I've also revised the a2x manpage (and tried to improve the intial explanation since it was never quite accurate in the first place: one always could redefine W3M and LYNX to point to different programs...) I hope this slowly converges to "solved" ;-). your opinion?

elextr added a commit that referenced this pull request May 15, 2014
Modified the way the command string is constructed for calling the text-browsers
@elextr elextr merged commit ddb0947 into asciidoc-py:master May 15, 2014
@elextr
Copy link
Contributor

elextr commented May 15, 2014

Sorry for the merge delay.

@jghub jghub deleted the a2xfix branch May 15, 2014 12:30
@jghub
Copy link
Contributor Author

jghub commented May 15, 2014

no problem. happy to see it accepted.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants