1.0.0 roadmap

[aclist]

New API is working and represents a breaking change, thus major version must be bumped to 1.0.0. This is a good milestone to clear out general administrative issues:

Docs

• [x] Create an issue template for the bug tracker
• [x] Update documentation describing new manifest scheme
  • [x] Port existing docs template
  • [x] Document manifest keys
  • [x] Document PR requirements
  • [x] Add internal anchor links
  • [x] Document mutation observer
  • [x] Expand TODO sections
  • [x] Add admonition about global scope collisions

Cosmetic

• [x] Fix the bug mentioned in #26 and appearance of the button
• [x] Restore hover color when rolling over links in the list; preferably rolling over the ACTIVE page or mod will not change its color so the active page remains distinct
• [x] Finalize working name, cf. https://github.com/aclist/kbin-megamod/discussions/15
• [x] Full screen, responsive modal
  • [x] Bug: width varies when selecting pages
  • [x] Bug: docked mod height is variable (consider dropping)

Code

• [x] #12 (deemed WONTFIX at this time)
• [x] Add one-shot and recurring modes
• [x] Attach second mutation observer to post events
• [x] Merge and integration test minimum collection of scripts
  • [x] mail.user.js
    • [x] must update item-link in place if it exists
    • [x] mail links must point to own instance instead of hardcoding
  • [x] label.user.js: not working at all
  • [x] dropdown.user.js must set up and teardown correctly
  • [x] collapsible comments triggering endless loop when called by megamod's mutation observer
  • [x] refactor syntax highlighting
    • [x] support namespaced config syntax
    • [x] clean up object to remove redundant URLs
    • [x] don't reinject header
    • [ ] clipboard button is broken on firefox
  • [x] Remove debug code

Cleanup

• [x] Rename repo to KES
• [x] Scrub references to "megamod" from script
• [x] Update topside screenshot
• [x] Audit stale issues
• [x] Add documentation to live page
• [x] Update README with documentation link

[artillect]

I took a look at the issue with the collapsible comments, but I'm not sure how to fix it. Even with it disabled, there's an infinite loop, so I'm inclined to believe something else is going on, but if it appeared after the collapsible comments script was added then it's probably an issue with that

As for the mail, it'd be nice if the script set the innerHTML of the link instead of innerText, so you can add things like fontawesome icons or format the text however you want

[artillect]

I think I can pretty easily fix the issue with the width changing when switching pages, and the height changing while docked

Hmm, the infinite loop seems to have stopped now that I've had it disabled for a bit, I'll mess around with the collapsible comments script and see if I can figure out the cause

Edit: Never mind, it's back. I have no clue what's going on

[aclist]

I believe infinite loop is related to the fact that the master script is calling mutation observer now (see #35). We need to talk about this spec and whether this is sane. The point is that the master script watches for changes to '#content' and then triggers the mods when this updates, such as when infinite scrolling occurs. I implemented this because I think it should make it easier for the individual mods to not have to implement their own observers every time, so there is just one.

I believe infinite scroll is the main reason we need mut obs, right?

The thing is, collapsible comments (among other mods) modifies this content area, triggering a DOM change, which in turn triggers the mutation observer to callback and apply the scripts again, causing an infinite loop.

I solved this in one of my mods by simply returning with no action if the mod doesn't need to be applied. (I.e., toggle state was true but mod was already active on the page.) Otherwise, the mod will try to apply changes to that area, in turn triggering a mutation again, and so on infinitely.

So we need to defensively design around this.

1. Is it really a good idea to have each mod spawn its own (possibly multiple) mutation observers for the sake of infinite scroll?
2. If using a single observer, how can we refactor the mods so that they don't keep triggering when unnecessary? I simply did it as follows:

//toggle is ON
//mutation occurs
//script.js is called
//normally, script.js expects to create or destroy some element
//added a check to the top of script.js to just quit if the element has already been added:
if(element) { return }

3. Should we be watching '#content', or some other element?

For the mail script, not sure if you have seen the latest version, but currently I have a toggle between a text label and a fontawesome icon. Seems this could easily be extended if you want to choose more fancy styling.

[artillect]

I think that makes sense to me. I haven't really minded adding my own mutation observers to my scripts, since it isn't necessary for a lot of them. I'm not sure what advantages there are of using a single observer for everything, but I think it might be better to leave that up to each mod. Mostly for the sake of porting, I think we'd be better off if each script more or less "just works" after you've created the entry point.

In the meantime, I'll see if I can fix the infinite loop with this new knowledge

[aclist]

Well, the reason I thought it might be good to add is because I only noticed that kBin has lazyload recently, so realized I had to refactor a bunch of my mods to support this infinite scroll. I thought it might be good if the master script supports this out of the box so that the mods don't need to do extra steps. I also think there is a performance argument to be made here against having so many listeners. I agree that it only affects scripts that specifically update the thread content area.

I can take a look at it as well later today, haven't really delved into the logic of how the comment folding works. Perhaps you could explain the workflow in pseudocode

[artillect]

That's a good point, but I think there'd also be a performance penalty from refreshing each mod every time something changes. It could be configurable per-mod, which'd help out with that.

As far as how the collapsible comments mod works:

• Get the level of each comment from its class
• Starting from the bottom of the comments, iterate through each pair of comments until a parent-child pair is found by comparing the child comment's level to the possible parents' levels
• Insert the child into its parent's children div

And when you collapse a comment, it hides its contents and its children, which is much simpler than the algorithms that other scripts use to crawl the trees to collapse a comment's children.

In short, it completely rearranges the comments section, so it makes sense why it'd trigger the mutation observer. It has its own mutation observer (with a different callback function) to check for new replies that you make.

[aclist]

That's true. Is there any scenario other than infinite scroll when the page contents update?

[aclist]

Compromise solution: use the manifest to specify whether mods are one-shot or recurring.

• On page load, call the normal init function that triggers every script.
• When the mutation observer triggers, rather than applying every script monolithically, check if the mod is of type recurring and only apply those
• Mods don't need to call their own observers

Does this seem reasonable?

Or is it more likely to be the case that mods already have their own observers and refactoring them is annoying? I can't say offhand the longterm overhead penalties of having too many observers.

[artillect]

That's true. Is there any scenario other than infinite scroll when the page contents update?

Making/editing a comment, and any interactions added by scripts that modify the page (like collapsing comments)

I'm not sure how many scripts will be ported vs written directly for this in the future, but I think having the one-shot/recurring config is perfect.

[aclist]

Right, I think it is true that this could affect porting/adoption insofar as it means scripts that have their own mutation observer have to be changed a bit. But are those types of scripts in the majority? Is it reasonable to enforce our own method? Is it not true that we would have to do some slight touch-up of scripts that people submit anyway? Are we committed to just writing our own library of scripts and "porting" ones we find that we like?

I'm mainly just looking to make the internal framework as extensible as possible and offloading the setup of mostly everything into the manifest, which I think is the right thing to do here.

[artillect]

True, I think it's definitely a good idea to try to handle as much as possible in a central place, I'm just not sure how we balance making porting easy vs the performance of using a single mutation observer. I think it'll work well with the added config options though, that should prevent any big issues.

I have noticed though that the mutation observer triggers on every character you type into some text boxes, and every time you expand a post, that could cause some issues. Doesn't seem to really be causing any responsiveness issues but it could be a problem in the future.

[aclist]

Currently it's just triggering on changes to the entire '#content' area, but could use a more granular element if necessary (test if post/comment)

[artillect]

Do you have a branch for the documentation? I could help out with writing it, maybe it'd be good to add a wiki to the repo

[aclist]

Looks like I was wrong about the keepalive thing, I think some other script is interacting here and causing the mutation to occur twice. I've set aside some time to look at it later. I'm optimistic that implementing the one-shot/recurring thing should cut out a lot of the noise and make it easier to debug this. It's almost surely one of my mods that is to blame...

As for the documentation, it's going to be a standalone templated document, but I haven't commited any of the local work yet. It will follow the format used here: https://aclist.github.io/modtui/modtui.html. It shouldn't take more than 30 minutes to finish the first draft, just got some real world obligations to finish first.

I really appreciate your help thus far; I think it's going to be super easy to slap in new mods once we have this working, so hopefully we can soft launch next week after debugging the rest?

[artillect]

Looks like I was wrong about the keepalive thing, I think some other script is interacting here and causing the mutation to occur twice.

That'd make sense. I tried looking around to see what was getting modified but I didn't see anything, maybe there's something I missed.

As for the documentation, it's going to be a standalone templated document, but I haven't commited any of the local work yet.

Sounds good, I'm ready to lend a helping hand once you've committed it. I've gotten pretty good at writing documentation over the last few years.

I really appreciate your help thus far; I think it's going to be super easy to slap in new mods once we have this working, so hopefully we can soft launch next week after debugging the rest?

Thanks, and thank you for creating this! I can't wait until we're able to get a bunch more script authors working on adding features to this. I've been on the lookout for bugs, and I'll let you know if I notice any more.

One thing I did notice while working on updating my mods to work with the new settings is that checkboxes don't seem to work properly. I might be misunderstanding how they work, but it seems like they're checked if either the value or checked properties are set. I tried to add some code to handle the checkboxes but I can't seem to get it working. Where are all the places that I would need to add code to handle that, other than the big switch statement with the input types?

[aclist]

Are you talking about adding a custom field of type checkbox to a mod's options? Or are you talking about the toggle itself? I dropped the syntax type: checkbox from the manifest because all mods have a base toggle by default now, so additional types should go under the fields array:

    "fields": [
      {
        "type": "radio",
        "initial": "Hide logo",
    "label": "Logo type",
        "key": "logotype",
        "values": [
          "Hide logo",
          "Bird logo",
      "3D logo"
        ]
      }
    ]

Or are you just saying that custom fields of type checkbox don't work at all? I didn't exhaustively test every field type.

The toggle state (true/false) of the mod itself is stored under the megamod-settings key of localStorage:

Storage { changelogo: '{"logotype":"Hide logo"}', 
timestamp: '{"offset":"Local time","state":"on"}',
"megamod-settings":'{"addMail":true,"initMags":true,"magInstanceEntry":true,"hideDownvotes":true,"hideUpvotes":true,"updateTime":true}', 
mail: '{"type":"Text","text":"PM","state":"on"}', length: 4 }

If you set a custom field of type checkbox and give it a key name under a namespace, the resulting value should be stored under namespace.key and the value should, I think, by default be passed as "on". You can see how this works by toggling the "Add mail icon" mod between Text/Icon and looking at the resulting localStorage.

But it may be that I forgot to explicitly test checkboxes.

[aclist]

other than the big switch statement with the input types?

Shouldn't be any place else. Only radio/select have special logic because I expected all other types to emit a single value

[artillect]

Are you talking about adding a custom field of type checkbox to a mod's options? Or are you talking about the toggle itself?

Sorry, I should've specified, I meant adding a custom field with the type checkbox. I'm adding checkboxes to enable/disable different things within my mods. It writes the initial value to it, but since checkboxes consider having a value as being checked, it doesn't behave exactly the same as the other inputs. I'll see if I can get it working, but I can work around that using the other types.

[aclist]

I see, well, no reason not to add support for that, then.

[aclist]

It should be working as of the latest commit.

Snippet from the manifest. Adds a new key called checkbox_label:

     "namespace": "checkbox-test",
    "fields": [
      {
        "type": "checkbox",
        "label": "Checkbox follows below",
    "initial": false,
    "checkbox_label": "Setting 1",
        "key": "box"
      },
      {
        "type": "checkbox",
    "initial": true,
    "checkbox_label": "Setting 2",
        "key": "box2"
      }

label is just the descriptive label printed above a custom field and might be unnecessary with a checkbox. checkbox_label is the custom text printed adjacent to the checkbox.

Now, when checkbox is toggled, its state is passed directly into the localStorage, like other types. At some point we'll probably want to refactor the switch statement to be less verbose.

[aclist]

OK, added initial documentation to /docs. There are a few areas marked TODO that need expanding upon.

I think the mut obs loop seems to have fixed itself after I added the recurs key to the manifest

[aclist]

Take a look at this:

// ==UserScript==
// @name         mut test
// @namespace    http://tampermonkey.net/
// @version      0.1
// @description  try to take over the world!
// @author       You
// @match        https://kbin.social/
// @grant        none
// ==/UserScript==

function mymut(list, observer) {
    for(const mutation of list) {
        console.log(mutation);
        if (mutation.type === 'childList') {
            console.log('A child node has been added or removed.');
        }
        else if (mutation.type === 'attributes') {
            console.log('The ' + mutation.attributeName + ' attribute was modified.');
        }
    }
};
     const watchedNode = document.querySelector('.entry');
       const obs = new MutationObserver(mymut);
        obs.observe(watchedNode, {subtree: true, childList: true, attributes: false });

1. Add this script
2. Disable all other scripts
3. Enable infinite scroll
4. Refresh the page
5. Clear the console
6. Scroll down to the bottom once to trigger infinite scroll
7. Idle on the page for a few minutes
8. Console gets filled up with 2-3 mutations
9. Inspect contents of target

Root cause seems to be due to the fact that the "timeago" is constantly updating to show the post age over time, thus causing small mutations that are false positives.

Seems like we would have to filter these and ignore if the mutation is of the timeago className, but one problem I see is that when doing the infinite scroll, one of the first mutations to hit the list is the timeago, rather than the element itself. It's extremely expensive to walk through the entire list until a valid change occurs, so I think we need to just be more granular about which element we are targeting so that we can get actual, valid child entries rather than these timestamps.

[aclist]

Here's what I have worked up so far:

function initmut(list) {
    for (const mutation of list) {
        if (mutation.target.className === 'timeago') {
            console.log("Invalid mutation, discarding");
            return
        } else {
            console.log('Valid mutation');
            for (let i = 0; i < json.length; ++i) {
                if (json[i].recurs) {
                    applySettings(json[i].entrypoint);
                    obs.takeRecords();
                }

            }
            return
        }
    }
}

const watchedNode = document.querySelector('[data-controller="subject-list"]');
const obs = new MutationObserver(initmut);
init();
obs.observe(watchedNode, {subtree: true,childList: true,attributes: false});

This seems to work and discards timestamp mutations, so won't proceed to apply mods otherwise. This only works on the top threads index, but I think you can attach multiple nodes to the same observer, or we could create a separate observer for the posts.

I wasn't able to find a good invocation of elements that would find changes but exclude the timestamps altogether when querying selectors; maybe you can think of something. It seems like it might be a necessary evil of the persistent observer to have to listen for and discard these timestamp events. Not sure yet.

[artillect]

Thanks for getting the checkboxes working! I'll take a look at the docs and try to expand upon the TODO sections that I can.

Of course it was the timestamps causing that, makes sense. I'm not sure how we should handle that, but I think the current solution should work for now, it'll probably just require some workarounds to get mods that mess with the timestamps working perfectly with infinite scrolling.

Regarding the TODO in the docs about the author name link, I believe relative links should work. So instead of linking https://kbin.social/u/shazbot, it'd be /u/shazbot@kbin.social. Should be a pretty easy fix, I can go ahead and do that.

[aclist]

Regarding the TODO in the docs about the author name link, I believe relative links should work. So instead of linking https://kbin.social/u/shazbot, it'd be /u/shazbot@kbin.social. Should be a pretty easy fix, I can go ahead and do that.

Does this work for sending messages as well?

[artillect]

Just used the HTML inspector to quickly test it out and it looks like it does

[aclist]

Across all instances, or only kbin ones?

[artillect]

I'm not sure if cross-instance messaging is working yet at all but yeah that should work in principle, the link would be /u/user@instance.tld/message/

[aclist]

I see, thank you

[artillect]

I tried manually going to the message page for someone from fedia.io since the button doesn't show up on their profile, and it just takes me to the home page so it definitely seems like it isn't working

[aclist]

I'll change it so that mail links point to one's own instance based on the current window location (assume browsing from within the same instance). This is kind of cruddy, but is the best we can do for now

[artillect]

I'm making some edits to the docs, and I think we need to emphasize the fact that this is both a configuration menu and a suite of modifications. Right now the non-developer sections of the documentation sound very programmer-y for lack of a better word, but then again, it might be better to leave the simplified version for the readme. I'll make some edits to the first few sections, feel free to drop them or make further modifications

[aclist]

I tend to leave the bare minimum explanation for end-users in the README with a link to the docs (easier to maintain a standalone doc than updating the README or making a wiki), then a basic summary in the docs at the top, then leave the rest of the documentation for developers, i.e., no need to read further once you get it installed.

Of course, it also makes sense to have two documents, but I can't really think of too much in-depth explanation a user needs besides install and bug report. I agree that I didn't put much thought into the summary text at the top, I was just trying to get some basic sections down.

I assume the actual friendly introduction to "what this is" would take place on kBin itself rather than in the document. (You're in charge of writing that post, btw :D)

[aclist]

I think I got the strictest implementation of the mutation observer working now, catching only valid new threads and new posts, and without the need to filter extraneous noise from timestamp events, because the subtree is excluded. Everything seems to be updating correctly now with infinite scroll and on creation of new comments. The only one that isn't behaving well is the collapsible comments, in that the lines aren't being drawn immediately when a new reply is made.

[aclist]

I think I can finish the documentation today. I cleaned up all of the scripts, so they should be working.

We can make a separate thread for this, but:

• Should be in a brief code freeze after 1.0.0 with no new features elaborated (waiting on bug reports from users)
• The copy to clipboard button on the code syntax highlighting mod is not working on Firefox. Does it work on your browser?
• For some reason, the master script does not work when encapsulating it in its own sandboxed GM function (the boilerplate use 'strict' function provided when creating a new userscript)
• Need to draft and proof a kbin post
• Need to test all mods individually and in conjunction with each other and see if anything weird is happening (looks good over here)
• Determine whether the collapsible comments implementation is "good enough" for now

[artillect]

Sounds great!

I tested out the clipboard button and it is working for me, but the title bar shows up twice (maybe because I'm not completely up to date with testing). As for the sandboxing, would that cause the main script to be unable to see the other scripts? I'm not 100% sure how that works exactly. I'll get to work writing up a post, I'll post the first draft in the issues once I've finished it.

I'll keep testing out the mods too, I'll just turn basically all of them on and see what breaks. I'll also see what I can do about fixing the various issues with the collapsible comments

[aclist]

I did find one small bug just now, which is that the mod I made called "Verbose timestamps" actually needs to be informed of timestamp change events so that the verbose timestamps don't get overwritten when timeago changes, which happens after every passing minute, apparently (kind of a strange feature on the site--I would expect the time ago to just show you when it was posted relative to when you reloaded the page, not have a constant live tick). But it also needs to be applied in the usual case of infinite scroll along with other mods, so can't be calling its own observer or it will cause a loop. So I think I might make a special exception for this script and attach a watcher in kes.user.js that only calls that script in the event of timeago changes. Presumably there won't be many scripts that change the timeago field.

I was able to drop all mut obs watchers from other mods, though, as the watcher that kicks off at the beginning of the master script is enough for all of them to be triggered when needed.

I tested out the clipboard button and it is working for me, but the title bar shows up twice (maybe because I'm not completely up to date with testing).

It is fixed if you pull the latest version. There are a number of changes in there, particularly with getting mods up to date. I also implemented #54. Clipboard may be a(nother) Firefox thing.

You should also clear your localStorage for megamod-settings, since everything is being called "kes" now: localStorage.removeitem('megamod-settings')

As for the sandboxing, would that cause the main script to be unable to see the other scripts?

As I understand, encapsulating the whole script as a private function of the outer function binds it to GM's context instead of the entire window context. (More secure).

As a second issue, strict mode: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode

However, strict mode has semantic differences and might not interpret some of the current code correctly. In addition, would have to audit the sub-scripts being sourced in the includes to see if they set strict mode: https://www.tampermonkey.net/documentation.php?locale=en#meta:require

Note: the scripts loaded via @require and their "use strict" statements might influence the userscript's strict mode!

But I don't think any of the scripts in /mods are, we pretty much scrubbed them.

We may be able to do one or both of the above. Incidentally, on this subject, and to your prior comment, I don't think custom fields should expose the ability for users to style content with raw HTML (innerHTML) versus change innerText or color, etc. They could insert anything (untrusted code received from somebody) into the HTML and it seems rife with problems.

I'll keep testing out the mods too, I'll just turn basically all of them on and see what breaks. I'll also see what I can do about fixing the various issues with the collapsible comments

Also look for:

• When a mod is on, and one of its custom fields is updated, does the change override the old setting in-place?
• Changing a custom setting while a mod is off and then turning it on again: does the previously applied change take effect?
• Infinite scroll

[aclist]

Forgot two other things:

• Changed the keybinding to Ctrl-Shift-L only (show/hide modal with single binding). This may or may not be good
• https://www.tampermonkey.net/documentation.php?locale=en#api:Subresource_Integrity -- should be a good way of signing the resources as the correct ones and could open the path to bundling remote mods if they are bound to a hash (undecided on this). Only supported in TM though afaik

[artillect]

I've been testing out the new updates, and everything looks good to me so far. One issue I have noticed is that the ctrl-shift-l binding seems to open a new tab from the contents of your clipboard on Edge.

I've linked my draft for the release post in #59, and I think it's pretty much complete. Feel free to make edits if you'd like, you should be able to without logging in. I think the title could use a bit of a change, but I'm not sure what to change it to.

Do you think KES will be ready to post tonight on /m/kbinstyles, to get some fresh eyes on it and do a bit of a quieter release before posting it on /m/kbinmeta later once we've ironed out any major issues that pop up?

[aclist]

Great job, thank you. I am making a few last-minute changes:

• [x] Change keybinding to Ctrl-Shift-U. Hopefully this doesn't conflict? It's hard to find open keys these days.
• [x] Restore ESC keybinding
• [x] Proof docs and push to live site
• [x] Approve final PRs and merge into main
• [x] Bump version to 1.0
• [x] Proof the document you shared (just gave it a cursory read and looks great, I'll probably just add a couple lines)

I suggest pre-seeding /m/enhancement with at least one guidance post (e.g., redirecting to the issues page or the repo top page or giving some admonition about how to request features), so that it's not just an empty forum. I'll leave that to your discretion.

I'll give you a heads-up here after I do that

[artillect]

Hopefully this doesn't conflict? It's hard to find open keys these days.

Unfortunately it does lol, that opens the screen reader in Edge apparently. Finding a free keybinding is definitely made difficult by all of its feature bloat. The rest of the changes sound good though.

I've made a couple of pinned posts to /m/enhancement to make it seem a little less empty. Good call on that. I'll wait for your heads up!

[aclist]

Hmm, that's annoying, maybe we need to bring F- keys into the mix here?

[aclist]

How about Ctrl-Shift-?

[artillect]

That toggles the sidebar, but Ctrl-Shift-H seems to be free. It looks like there must be some way to override the browser, when you use the shortcut Ctrl-Shift-K, on other pages it refreshes, but on GitHub it opens the "command panel" or whatever it's called.

[aclist]

Ctrl-Shift-H is history on Firefox, lol

I think you can use event.preventDefault(), although it seems inherently wrong to block default functionality in someone's browser. Depends. I'll make a change

[aclist]

Try the latest version on testing and see if it works with Ctrl-Shift-? now

[artillect]

That works for opening it, but not dismissing it

[aclist]

Due to the way the array is built up with combined keypresses, make sure you don't have the ctrl-shift keys held down when trying to hit ? again. Invoke Ctrl-Shift-?, let go of everything, then push that again

[artillect]

Ah, there we go, normally people won't be opening and closing it quickly like that so I think that's perfectly fine

[aclist]

It is tech-debt in my mind, should be tweaked a bit in the future but it is a known kludge