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

new TeX mode (again) #625

Closed
wants to merge 1 commit into from

Conversation

matthias314
Copy link
Contributor

This patch (hopefully) improves Aspell's TeX mode. It is practically identical to the one posted to the Aspell mailing list in 2011.

New features implemented by this patch

  • Aspell skips over math content inside $...$, $$..$$, \(...\), \[...\] and common LaTeX and AMS-LaTeX environments like equation and gather. Additional environments to be skipped over can be defined via the new tex-ignore-env list. Otherwise math formulas trigger a huge number of false alarms.

  • Forced spell-checking of macro arguments while skipping over arguments or environments. This is achieved with the new T option to add-tex-command. Remarks added in 2022 about the T option:

    • I guess the T was supposed to mean "toggle". Maybe F for "force" would be a better choice.
    • Would it be better to make this the standard behaviour of the P option? Asked differently: What LaTeX macros are there using P where we don’t want the corresponding argument to be spell checked in math mode? Such a macro should also have arguments without spell checking because otherwise there is no need to define it in Aspell. I can only think of \textcolor{color}{text} from color.sty (where text can also be in math mode!).
  • Discretionary hyphens and italic corrections are ignored. For example, hy\-phen and shelf\/ful are recognized as single words.

  • Spell-checking can be manually switched on and off by putting the text aspell:on and aspell:off into the file. This allows to skip over macro definitions and other parts of text that confuse Aspell.

See the Info file for more details.

Comments about the changes

./modules/filter/context.cpp
It seems that the order of the context filter was undefined. Whether one wants to have this filter before or after other filters depends on the application in mind. I need it before the tex filter so that one can place the aspell:off and aspell:on flags inside TeX comments.

./modules/filter/tex.cpp
The new code for the tex filter.

./modules/filter/tex-filter.info
The new default settings for the filter variables. tex-ignore-env is new. Most changes to tex-command are related to the new T option. The new begin line is used internally. The rest are additions and corrections to existing definitions.

./modules/filter/modes/tex.amf
Note that we enable the context filter by default.

./manual/aspell.1
./manual/aspell.texi
Updated documentation.

@kevina kevina mentioned this pull request Jun 16, 2022
@dkwo
Copy link

dkwo commented Jun 20, 2022

Thanks for your work.
I'm unable to apply this patch cleanly on 0.60.8
Is there something I'm missing?

@matthias314
Copy link
Contributor Author

@dkwo: There have been several (more or less) recent commits fixing typos in the documentation, the latest being #624. They can be ignored. Are there any conflicts regarding the code?

@dkwo
Copy link

dkwo commented Jun 20, 2022 via email

@matthias314
Copy link
Contributor Author

@dkwo: That's strange. I've downloaded the patch and the 0.60.8 tar ball. Then

/tmp/aspell-0.60.8$ patch --dry-run --verbose -p1 </tmp/625.patch

gives (among other things)

checking file modules/filter/tex.cpp
Using Plan A...
Hunk #1 succeeded at 32.
Hunk #2 succeeded at 53.
Hunk #3 succeeded at 71.
Hunk #4 succeeded at 99.
Hunk #5 succeeded at 267.
Hunk #6 succeeded at 294.

@dkwo
Copy link

dkwo commented Jun 21, 2022

My bad, sorry about that. I downloaded the patch again, and now it applies cleanly, if I remove the manuals part.
It seems to work fine here, but I have one question.
Suppose I want to ignore the \cref command (similar to \eqref and \ref): what is the correct synax to pass aspell?

@matthias314
Copy link
Contributor Author

To ignore \cref{label}, you put

add-tex-command cref p

into your .aspell.conf file. One could also add a corresponding definition to modules/filter/tex-filter.info, and for many other macros as well.

@dkwo
Copy link

dkwo commented Jun 21, 2022

Thank you.

@dkwo
Copy link

dkwo commented Apr 25, 2023

I've been using this for almost a year, can confirm it works well.
@kevina any chance it can be merged?

modules/filter/modes/tex.amf Show resolved Hide resolved
@@ -63,6 +63,7 @@ namespace {

PosibErr<bool> ContextFilter::setup(Config * config){
name_ = "context-filter";
order_num_ = 0.15;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the order number is likely to break something. If it needs to be changed than it should be an option to the context filer, something like "order-num <float>" where float is a number between 0 and 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At present, order_num is not defined at all for the context filter. What does that mean? Is the filter applied first, last or somewhere else? (If I knew the answer 12 years ago when I wrote this patch, I've forgotten it in the meantime.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It defaults to 0.50:

IndividualFilter() : name_(0), order_num_(0.50) {}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kevina I understand your concern that this may break something. For the new TeX mode, however, it is important that the context filter comes first. So it would have to be added as an option. Unfortunately, my familiarity with the aspell code is (and never was) sufficient to add such an option. Would you be willing to help out?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. It shouldn't be too hard to modify the context filter to add the option.

@kevina
Copy link
Member

kevina commented Oct 20, 2023

@dkwo @matthias314 my main concern with this is causing a regression. I would be more willing to accept this if it has some test cases. There is now a primitive framework for this is the tests/ directory (see the filter-test target in the Makefile). Ideally I would like to first accept a pull request with some tests for the existing TeX filter to make sure this new filter doesn't break anything.

@kevina
Copy link
Member

kevina commented Mar 14, 2024

@matthias314, I am leaning towards accepting this once the above issues are resolved.

@kevina
Copy link
Member

kevina commented Mar 14, 2024

To be clear by above issues I mean the two comments in the code review, tests would be nice, but not really required.

@matthias314
Copy link
Contributor Author

I'm very sorry for my belated reply.

Regarding a potential regression: That's a valid point. I've been using the new TeX mode for many years now, but only on a very limited variety of LaTeX documents. By the same token, I think that if I got around to add test documents, then they would only cover a tiny fraction of the kinds of documents other users have. Would be a possible alternative to add the new TeX mode besides the existing one so that users could fall back to the old version in case the new one causes problems?

@kevina
Copy link
Member

kevina commented Mar 14, 2024

Would be a possible alternative to add the new TeX mode besides the existing one so that users could fall back to the old version in case the new one causes problems?

I thought about that, but rejected the idea as being too complicated, but that was many years ago. I'll have another look sometime soonish. Hopefully by the end of the weekend.

@kevina kevina mentioned this pull request Mar 16, 2024
3 tasks
@kevina
Copy link
Member

kevina commented Mar 16, 2024

Closing in favor of #644

@kevina kevina closed this Mar 16, 2024
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.

3 participants