Closed Pamasich closed 8 months ago
Pamasich
commented
8 months ago Currently planned goals:
Then:
Pamasich
commented
8 months ago fix-codeblocks works correctly, including hiding the original code block. However, the syntax highlighting mod adds a header as a separate element, it does get displayed, and doesn't interact with the fixed codeblock. Should look into why that is and what would have to be changed in that mod to make them compatible, if even possible. Alternatively, if I can't get them to work together, this should hide the header for fixed code blocks and a note should be added to the description.
Pamasich
commented
8 months ago There seem to be two issues here:
@aclist To fix the second point I'd probably have to manually run the syntax highlighting mod again after fix_codeblocks is done. Is this something that you would rather I don't do? Running a mod from another mod I mean. I can just add a note that the two are incompatible.
aclist
commented
8 months ago In theory, load order should be configurable if we rearrange the order things appear in the manifest.
I don't fundamentally see a problem with mods calling other mods if it makes sense to do so, provided it doesn't cause unwanted recursions. But maybe ther is a better way? My concern would be how this affects teardown or if it would cause duplications.
Pamasich
commented
8 months ago Basically, the reason I didn't think of just changing the manifest order was because the load order seemed unreliable. Sometimes it applied the syntax highlighting to both, sometimes to only the original code block. It's like something was delaying the execution of the mod until a point after fix-codeblocks had finished, and that same thing could theoretically delay fix-codeblocks too if it's a KES thing. So I didn't see changing the order as a reliable solution.
But some more testing has shown that the mod just gets executed twice sometimes, the actual load order is consistent. So changing the order in the manifest should be reliable and the better idea. Can I just do that manually, or is the manifest automated with a script that would have to be edited?
Also, do you have an idea why it might get executed twice sometimes? The mod doesn't seem to register its own mutation observer, so a bugged mutation observer unreliably picking up fix-codeblocks creating a new code block isn't the reason.
My concern would be how this affects teardown or if it would cause duplications.
I think mods are already supposed to support being run multiple times, right? Isn't that what happens if a setting gets changed? As I wrote above, this one already seems to get run twice sometimes anyway; in those cases teardown properly removes the header (and I assume syntax highlighting too) from both code blocks, and the header on the original one doesn't get duplicated.
aclist
commented
8 months ago Yeah, I think this mod needs to be audited, because there is something weird about the control flow. On a quick glance, it looks like some functions have redundant branches (conditional statements doing the same thing?), and variables being set for seemingly no reason.
It's not out of the question that KES' mutation observer could be catching on the mod firing and updating the contents of a comment. The recurs bit is set to true for this mod, and the mutation triggers on mutation.target.id == "comments", but in theory this should cause infinite recursion, so might not be this.
But I suspect it's something more simple. Do you have a link to a thread with code blocks on it?
Pamasich
commented
8 months ago Do you have a link to a thread with code blocks on it?
aclist
commented
8 months ago Yeah, thanks, that is handy.
Pamasich
commented
8 months ago Here's one with more code blocks.
Interestingly seems like the syntax highlighting mod turns inline code into code blocks too. Which I just found out doesn't properly work with teardown. The CSS doesn't get removed from inline code on teardown.
aclist
commented
8 months ago Interestingly seems like the syntax highlighting mod turns inline code into code blocks too.
It seems to have to do with this:
function addPreTag (parent, placement, code) {
// For some reason, sometimes code isn't wrapped in pre. Let's fix that.
const pre = document.createElement('pre');
parent.replaceChild(pre, code);
pre.appendChild(code);
hljs.highlightElement(code);
}Ah, I was wondering what they meant with "sometimes code isn't wrapped in pre" when I looked at the code.
aclist
commented
8 months ago Should that responsibility be offloaded to the repair codeblocks mod instead? It seems like they are both doing double duty here and causing unnecessary collisions.
Pamasich
commented
8 months ago Should that responsibility be offloaded to the repair codeblocks mod instead? It seems like they are both doing double duty here and causing unnecessary collisions.
I'm not sure which responsibility you're referring to?
if you mean the addPreTag function, I honestly think it should be removed entirely. It seems like the author didn't realize that's actually inline code like this, which should be kept inline imo.
Pamasich
commented
8 months ago Interestingly seems like the syntax highlighting mod turns inline code into code blocks too. Which I just found out doesn't properly work with teardown. The CSS doesn't get removed from inline code on teardown.
Also, I just found out that's a general issue. I was sure I've seen it remove the CSS correctly earlier today, but that doesn't seem to be the case.
aclist
commented
8 months ago if you mean the addPreTag function, I honestly think it should be removed entirely. It seems like the author didn't realize that's actually inline code
like this, which should be kept inline imo.
Right, makes sense.
Also, I just found out that's a general issue. I was sure I've seen it remove the CSS correctly earlier today, but that doesn't seem to be the case.
It looks like it tries to remove kchInjectedCss on line 20, but that doesn't actually get initialized to a value at any point. It should be using the safe addStyle and removeStyle methods provided in safeGM.
I'm not going to be available for 48 hours or so, but will start refactoring it at that time. You can expect the addPreTag() to be removed, along with a cleaner startup and shutdown.
Pamasich
commented
8 months ago I probably won't be able to work on KES over the next few days either (and didn't get to this weekend)
aclist
commented
8 months ago I've started doing some local refactoring to the syntax highlighting mod. Being one of the earliest KES mods, it does several things in what would be considered an unidiomatic way by current KES standards, and also has some internal problems:
style.display attribute of a code block is none, since fix-codeblocks may have hidden the original block)I am using the pages below as tests for code blocks and generic inline monospace, respectively:
aclist
commented
8 months ago I briefly tried experimenting with having the code syntax highlighting mod directly call the repair codeblocks mod prior to applying its style, and while this works, it introduces a problem in cases like this one:
I've decided to introduce an option at the manifest level called depends_on. This allows mods to be in lock-step with each other, which I think is a better solution in the long term, since mods that behave synergistically with each other should either be both off or both on at the same time; allowing one to be out of step with the other can cause problems like the above.
Of course, some slight modifications to the runtime control flow of each mod are now needed, such that they check if certain conditions have been met (such as a class name or value set by the paired mod). In the new logic, load order becomes irrelevant, and KES handles calling the mods in sequence.
For the sake of argument, assuming syntax highlighting and repair codeblocks are adjacent to each other in the manifest, the effect is as follows:
If the syntax highlighting mod is applied first, it will check if its dependency has been applied, such as by checking if repair codeblocks set the code block ranges to "fixed". If this value is false, it will abort, and then its dependency (repair codeblocks) is applied.
Repair codeblocks applies the necessary changes and then KES applies its dependency. The dependency would find the fixed codeblocks this time, and continue applying the syntax highlighting.
In turn, when the iteration hits repair codeblocks, the mod will find that the fix is already applied and abort. Then, syntax highlighting (called as a dependency of repair codeblocks) will find that syntax highlighting was also applied, and abort. The second cycle is redundant, but exists to support scenarios where the user manually toggled on one mod before the other, since load order is non-deterministic here.
If repair codeblocks is applied first, it simply applies the necessary changes and then KES calls its dependency, which would find the fixed codeblocks and apply syntax highlighting. When it is syntax highlighting's turn, it would find the necessary changes and abort, and repair codeblocks would be called, which would also find the necessary changes and abort (redunant cycle equivalent to the prior example).
The state of both mods is set to ON at the user config level.
For the OFF condition, the inverse of the logic above applies. A user cannot turn off one mod without the other mod also being turned off, both visually and at the config level.
Feel free to punch any holes in this thinking.
Pamasich
commented
8 months ago This allows mods to be in lock-step with each other, which I think is a better solution in the long term, since mods that behave synergistically with each other should either be both off or both on at the same time; allowing one to be out of step with the other can cause problems like the above.
A user cannot turn off one mod without the other mod also being turned off
I disagree that this is a good idea. The two do different things and should be toggleable individually. It's just also important that they work as expected if they're both turned on, which is why I started this issue.
I think having two mods in lockstep like that is a weird idea anyway. At that point just combine them into one mod, a both-sided dependency is just a bigger mod with extra steps.
- Toggle syntax highlighting on
- Toggle repair codeblocks off
- Toggle syntax highlighting off, then on again
What's the issue that happened here? Is it not fixable by changing the code of one of those mods? I think if this causes issues, then that's a problem with one of the two mods.
The idea I had originally was to call syntax highlighting from the end of the repair codeblocks mod. Which should absolutely work without any weird issues like that. I do agree that an automated / standardized system is better than just wild west calling other mods, but making them requiring each other like that doesn't sound like the right idea to me.
aclist
commented
8 months ago Well, there are multiple issues to unpack here. The first is that some mods are actually a "feature", whereas others are essentially a patch. Repair codeblocks is a patch to something that is broken, but doesn't introduce new functionality per se. So it makes sense that some other, latterly-developed mods, might want to take advantage of this patch.
For example, suppose a mod is created six months after repair codeblocks was released. This mod modifies code blocks. Should every subsequent mod implement custom logic to call repair codeblocks? Ideally, there should be a codeless way to make a mod depend on some other mod without having to reimplement the same code. This does also raise the question of whether patches should be mods, but I'll put that aside for now.
There's also the issue that when one mod is created, some other use case that is later enabled by a new mod might not have been part of the original design of the first mod, so enabling inter-dependency would let you backport new features into an existing mod.
One concern I have with calling mods inline from one mod is that it may introduce infinite recursion, particularly if the mods mutually decide to call each other and this is not audited correctly. Syntax highlighting needs to call repair codeblocks first to sanitize code before making changes. But repair codeblocks (probably) needs to reapply syntax highlighting whenever it is toggled. But this triggers repair codeblocks to call syntax highlighting. Ad infinitum.
depends_on could actually be more granular, so that the mods are not literally in lock-step: you could have depends_on and depends_off, and the syntax highlighting mod sets repair codeblocks as its depends_on mod, whereas repair codeblocks sets its depends_off mod as syntax highlighting. If repair codeblocks is off, syntax highlighting should not be on, because syntax highlighting does not function correctly without it. But it is not a requirement of repair codeblocks for code to be highlighted. By contrast, if syntax highlighting is on, repair codeblocks must first be on, otherwise the highlighting will be applied to the wrong blocks.
Actually, the thread checkmarks mod is one mod that depends on another mod: the suggestions omnibar. This is because the logic for fetching the user's subscriptions is fairly complicated, and it seemed wrong to reimplement all of that for the purposes of adding checkmarks to subscriptions. Currently, the description for the thread checkmarks mod tells the user that they have to manually enable omnibar in order to use thread checkmarks. Granted, the logic to fetch subscriptions could be moved elsewhere (some globally shared function that resides outside of mods), but this seemed like a decent compromise. So thread checkmarks could also use depends_on.
The idea I had originally was to call syntax highlighting from the end of the repair codeblocks mod.
The problem is that there is no load order. The mods in the manifest are loaded in sequence, and whichever comes first is the one to go first. If syntax highlighting is applied first, it will find the "unclean" code block and apply to that, but then repair codeblocks will modify that block. The proposed solution here is to reapply syntax highlighting at the end, but if the changes have already been applied, you'd have to both teardown and setup syntax highlighting (more on that later).
Load order also seems ungainly to me, because now the mods have to be shifted around in place in the manifest, rather than being agnostic as to their literal location in the file.
What's the issue that happened here? Is it not fixable by changing the code of one of those mods?
I thought some visual explanation would be in order here, so I started taking before/after screenshots of both the page and the source, but it seemed confusing without additional context, since you have to compare the original markup, then the markup with only syntax highlighting applied, then the markup after repair codeblocks is applied to the previously modified block, then the code after one of them is disabled, etc. And I was having trouble fitting all of that in this comment.
The easiest thing to do here is:
display=noneSure, you could ask syntax highlighting to run again from within repair codeblocks, but there's also the issue of this erroneous header and "bash" being detected as the language, which is a lot of unnecessary cleanup. If you just recurse through syntax highlighting again, it's already too late, because the erroneous headers have been inserted. You would have to turn syntax highlighting off and then on again, or something. And by this point we are calling one mod three times (on, off, on) for the sake of compatibility...
Taking the opposie tack: as a test, I modified syntax highlighting to call repair codeblocks first, and this way everything is clean before syntax highlighting or any headers are applied, and the language (C) is detected correctly. Where things start to go haywire is if you turn the individual mods on/off after this, as this causes unpredictable behavior. If you turn repair codeblocks off and then on now, the highlighted code will revert to span tags, and then if you toggle syntax highlighting off/on again, it will just be applying to unsanitized code.
Sorry if this explanation is not intuitive. Like I said, this might come across better in photo/video, but I thought a slideshow of images in a GitHub comment would be worse than just describing it.
So either you modify both mods to apply each other inline, with requisite checks and abort conditions (trying to skirt the line to avoid infinite recursion), or you externalize the calling process to KES Itself.
What I liked about the way repair codeblocks is designed is that it neatly aborts if the blocks are already fixed. This makes it pretty easy to ensure that even if it's called via another mechanism, it won't actually do anything, since it handles its own checks.
Maybe the syntax highlighting mod doesn't have robust enough pre-setup checks, but the fact of the matter is that the syntax highlighting mod is actively missing a feature: repairing span tags before applying any changes. Technically this is a bug: the headers detect the wrong language because they assume span tags are part of the literal code.
The above covers situations where one mod depends on a fix implemented by another mod. But looking ahead to the future, there may be scenarios where a mod (Mod B) wishes to do something that "augments" an existing mod (Mod A), but the scope is too complicated (or mod authors cannot collaborate) to just grandfather that feature into the old mod. In theory, if we use both depends_on and depends_off, then Mod B can make sure Mod A is at least on in order to function, but the act of turning on Mod A alone will not forcibly turn on Mod B (i.e., not lock-step). It will just make sure that, at a minimum, Mod B is not on when Mod A is turned off.
Pamasich
commented
8 months ago Ah, I didn't see the header using the wrong language as a hard issue before. Yeah, then it makes sense to have the mod require repair codeblocks to be on. And the checkmarks mod also makes sense.
Though I would be in favor of ultimately moving the fetching of the subscriptions to KES itself rather than having every future mod that makes use of them require the omnibar to be turned on. But it works while there's only two of them.
But yeah, I do see the problem then. As long as it's not both mods requiring each other, I think it's a good idea then.
I'm not entirely sure though what the requires_off is for in this updated idea. I think all that's needed is that repair codeblocks is turned on and runs before syntax highlighting, which requires_on is responsible for. What's requires_off's role? Is it just to turn the syntax highlighting mod off if repair codeblocks is turned off?
aclist
commented
8 months ago If you turn off repair codeblocks while syntax highlighting is on, the syntax highlighting and headers stay the same, but the span tags get reinserted (with styling this time) into the existing highlighted range.
aclist
commented
8 months ago Though I would be in favor of ultimately moving the fetching of the subscriptions to KES itself rather than having every future mod that makes use of them require the omnibar to be turned on. But it works while there's only two of them.
I agree with that.
aclist
commented
8 months ago I've added a basic prototype of depends_on and depends_off to KES and updated the manifests for the repair codeblocks and syntax highlighting mods. The former sets the latter as its depends_off dependency, and the latter sets the former as its depends_on dependency.
depends_on and depends_off are arrays, with the order of dependencies to be loaded/unloaded listed in sequential fashion.
There are a few scenarios here. For the sake of brevity, I'll call repair codeblocks RC and syntax highlighting SH.
Using this setup, it's possible to atomically enable RC without enabling SH, but it's not possible to atomically enable SH without RC being applied. And any action toggling off RC will necessarily disable SH.
depends_on dependencies.This seems to be working correctly with the above link and with these mods in KES. Of particular interest is scenario A2. This works because mods tend to be well-designed when it comes to bailing out if they don't need to be applied again (for instance, in the case of recursion/infinite scrolling, where only certain elements on the page need to have a change applied again). If a mod continuously tries to apply its dependency after it has already been set on the page, this is liable to cause problems, so we have to make sure mods abort gracefully if there is no reason for them to reapply themselves.
As a future area for expansion, it may be interesting to list the dependencies inside of a mod's settings page, possibly with internal links to the dependencies.
Pamasich
commented
8 months ago As a future area for expansion, it may be interesting to list the dependencies inside of a mod's settings page, possibly with internal links to the dependencies.
Yeah, I think there'll definitely have to be some way of informing users that disabling one mod also disables another. Otherwise it'll just be confusing.
Does this resolve the incompatibility completely (in other words, can I close this issue) or is there something more to do? I assume it resolves it, but asking just in case I'm forgetting something.
aclist
commented
8 months ago I'll look into populating the mod pages with dependency information automatically, but if it is too complicated for now, will postpone it to the next release.
I believe this resolves everything completely, only two mods left to audit now and some general chores before 3.4.0.
I'm not sure if fix-codeblocks properly gets syntax highlighting applied to it. Or if that can even be done in an atomic manner.
Might have to consult with aclist. Possibly add a disclaimer to fix-codeblocks that it's incompatible with the syntax highlighting mod if it doesn't work.