Description

This moves all the functionality of the auto-mode into modable-buffer, which results in cleaner code and more reliable mode- and page- specific mode settings. What's new:

• define-auto-rule as a simple way to define arbitrary rules without touching the file with user rules.
• auto-mode renamed to auto-rules as a feature.
• Rule tests are now being evaluated, which means we can now put arbitrary lambdas inside the rules!
• Default rules for gopher, gemini, and all the internal pages that require it.
• apply-all-matching-auto-rules-p that allows applying all the matching rules instead of one (requested long ago by @hendursaga)!
• Minor fixes for no-procrastinate-mode, and others.
• I've reshuffled the file dependencies, and auto-rules.lisp (the renamed mode/auto.lisp file) now has access to buffer types and history, improving the typing and semantics of functions there!

What's removed:

• Numerous enable-modes doing the manual work.
• Lots of rememberable-p

FIxes #2064 Fixes #2004 Fixes #2066 Fixes #962

Discussion

1. It feels suspicious that I had to remove so many rememberable-p from the interface-related modes. The only modes that are left with (rememeberable-p nil) are the real one-shot modes. Maybe rename it to one-shot-p then?
2. Some of the slots from auto-mode feel weird in modable-buffer. Maybe there's a better place for the slots of auto-mode? I was unable to think of any...
3. I am now suspicious of any enable-modes, including the one in reduce-bandwidths-mode. Should we rather inherit the mode from the modes it enables, so that it's cleaner?
4. Should there be non-configurable default modes? Should gopher, gemini and internal page modes be redefinable from the auto-rules.lisp?
5. Should enable perform the work of make-instance automatically, or should it always be something more up to the one defining the method, like in repeat-mode?

Checklist:

Everything in this checklist is required for each PR. Please do not approve a PR that does not have all of these items.

• [X] I have pulled from master before submitting this PR
• [x] There are no merge conflicts.
• [X] I've added the new dependencies as:
  • [X] ASDF dependencies,
  • [X] Git submodules,

    cd /path/to/nyxt/checkout
    git submodule add https://gitlab.common-lisp.net/nyxt/py-configparser _build/py-configparser
    

  • [X] and Guix dependencies.
• [X] My code follows the style guidelines for Common Lisp code. See:
  • Norvig & Pitman's Tutorial on Good Lisp Programming Style (PDF)
  • Google Common Lisp Style Guide
• [X] I have performed a self-review of my own code.
• [x] My code has been reviewed by at least one peer. (The peer review to approve a PR counts. The reviewer must download and test the code.)
• [X] Documentation:
  • [X] All my code has docstrings and :documentations written in the aforementioned style. (It's OK to skip the docstring for really trivial parts.)
  • [ ] I have updated the existing documentation to match my changes.
  • [X] I have commented my code in hard-to-understand areas.
  • [x] I have updated the changelog.lisp with my changes if it's anything user-facing (new features, important bug fix, compatibility breakage).
  • [x] I have added a migration.lisp entry for all compatibility-breaking changes.
• [X] Compilation and tests:
  • [X] My changes generate no new warnings.
    • More so: the "Cannot open-code test the type HISTORY-ENTRY" is gone!
  • [X] I have added tests that prove my fix is effective or that my feature works. (If possible.)
  • [X] New and existing unit tests pass locally with my changes.

I've pushed a questionable change: (en|dis)able-modes now uses keyword arguments, including the :bypass-auto-rules-pthat's passed directly toenable/disable`. It breaks quite some modes. Investigating. In meanwhile, does it look like a reasonable move to you?

OK, it doesn't hang anymore, but repeat-mode doesn't seem to work (on master too :D). Investigating...

OK, it looks more or less complete (except for repeat-mode being severely broken everywhere, even on master). And there are many useful changes, too. @Ambrevar, @aadcg, @jmercouris, care to review?

Obs:

• In commit 53aca2d23 there are changes that seem orthogonal to its description (namely changes to small-web);
• Commits like 3c368fb4d, 0acb67334 or a0cbde45c should always be squashed in my opinion (since it's part of your "internal" reasoning).

A very welcomed change! Now I'm going to test this. I didn't use auto-mode before, so I need some time to get used to it :)

Obs:

• In commit 53aca2d there are changes that seem orthogonal to its description (namely changes to small-web);

No! The changes to small-web are exactly in line with the commit meaning. small-web-mode used to reproduce a lot of auto-rules functions, and now it simply uses auto-rules!

• Commits like 3c368fb, 0acb673 or a0cbde4 should always be squashed in my opinion (since it's part of your "internal" reasoning).

I don't see them being much of a problem, but I guess you're right :)

I don't see them being much of a problem

It's not a problem at all, it's just easier on the reviewers!

I believe you still haven't deprecated the rememberable-p slot from mode, correct?

Please review toggle-modes, https->http-loop-help and the config-fails-and-browser-restarts test.

In the docstrings and symbol names you seem to mix and match "previous" and "last", as well as "page" and "url".

When referring to "last URL", it might perhaps be better to say "last visited URL" to avoid abusing language and confusing the users.

If they do refer to the same thing, can we settle on a unique definition?

I believe you still haven't deprecated the rememberable-p slot from mode, correct?

No!

Please review toggle-modes, https->http-loop-help and the config-fails-and-browser-restarts test.

All done, except toggle-modes. Seems like some complex logic there, need to get back to it later.

toggle-modes done :)

Sorry for the delay, I'll get back to you as soon as possible!

To do: remove mode-invokation for good now that mode symbols are reliable mode representations.

OK, 6485dca removes mode-invocation, but I'm not sure that's the way I want to see it. @aadcg, @Ambrevar, @jmercouris, how does it look to you?

I've left some minor comments as requested. Overall I trust your judgement, since I don't have the big picture that you have, @aartaka.

@Ambrevar, time for a second round of review for this :) The most important question is: should we stay with mode-invocation objects, or use mere lists with enhanced typing, as in 6485dcaf723c13ec46d61b48b869769607c475f0?

I've also rebased over master. It works, it fixes the linked issues, it is mergeable (unless there is some more feedback on a better design it could have).

OK, will review when I'm done with #2536.

@aartaka could you please rebase over master so that I can review this?

@aadcg, done! CI is failing on CCL, but the functionality works just fine :)

@aartaka what do you mean by "rebased syntax" in "Fix the rebased syntax for enable-modes." (commit 47621f5)?

I've cut the number of commits significantly.

Commit e1cf9ef4e should be reworded - what exactly is broken? The explanation should be added below the commit top message.

Let me learn how to use the new version of "auto-mode" and play with it for some hours and I'll comment on the functionality.

Sure!

Commit e1cf9ef should be reworded - what exactly is broken? The explanation should be added below the commit top message.

I've reworded it in 59ed4e42b9f5a2192be82ba78d9978cfd08cb26a.

mode/repeat: Make it most correct code possible (still BROKEN tho).

repeat-seconds is still broken. It was broken on master too, so it's not a fault—and not a part of the scope—of this patch, but it's still annoying.

What's repeat-seconds? Perhaps you meant the repeat-every command?

It's still not clear what exactly is broken. Perhaps an example could be provided.

Suggestion:

mode/repeat: Refactor repeat-every.

It's still unreliable for this and that reason.

Should this feature be baptized "Auto rules" or "Auto-rules"?

@aartaka see c3a7df4bf, as I've re-wrote the manual section.