search

aprzn123 / TheThirdCan

A browser extension designed to improve https://twocansandstring.com
MIT License
7 stars 4 forks source link

Adapted everything for InjectToggleableScripts #36

Closed aprzn123 closed 1 year ago

aprzn123 commented 1 year ago

I think this will really help improve the code once the TODOs are resolved. Most things are working already, except for injected scripts relying on third. That can be for a future PR, though. Theki, can you take a look at this?

roxwize commented 1 year ago

These are my two cents on everything at the moment. I'll see if I can have a closer look after school is out and I'm able to access my computer. I apologise for the sometimes-counter-intuitive code.

aprzn123 commented 1 year ago

lol touching like every file in the project because these are such broad code changes. @thekifake feel free to take another look when you have a moment.

aprzn123 commented 1 year ago

It should all work right now, this isn't just the groundwork.

I haven't tried out the alert function, I was just adapting the stuff to what I added, we can switch to the custom alert next PR or something like that. Main difference between third and fourth is that third is meant to be accessed by src/js/*, and so has functions like InjectScript, whereas fourth is meant to be accessed by src/injected/*. This is why third.InjectToggleableScripts injects it.

The API for third.InjectToggleableScripts might be a little overly complex, which is probably what is leading to the confusion. It takes an Object where the keys are the names of config settings and the values describe what scripts to inject when the given configs are enabled. Each of those can be either just the name of the script, an Object of the form {name: "whatever.js"} and then whatever config options would be passed to the InjectScript function. It can also take a list of those, if you need to inject multiple scripts when the config option is enabled.

Let me know if you have any more questions, it's important that we all understand the codebase well.

aprzn123 commented 1 year ago

also not quite sure why we started using PascalCase for our function names, but I don't really mind.

HumanoidSandvichDispenser commented 1 year ago

I think we should keep it consistent with camelCase then. It's something I noted but forgot to mention.

aprzn123 commented 1 year ago

I feel like it might make sense, if only to further distinguish third and fourth functions from normal functions

roxwize commented 1 year ago

also not quite sure why we started using PascalCase for our function names, but I don't really mind.

Exported functions from modules, like many things, should use PascalCase in my eyes. It's a thing with Go, it's a thing with C#, it's a thing with Java, etc. It's more preference than anything, and you didn't really have to conform to it.

aprzn123 commented 1 year ago

I conformed to it because whatever we decide to do, the most important part is to be consistent.

roxwize commented 1 year ago

I haven't tried out the alert function, I was just adapting the stuff to what I added, we can switch to the custom alert next PR or something like that. Main difference between third and fourth is that third is meant to be accessed by src/js/*, and so has functions like InjectScript, whereas fourth is meant to be accessed by src/injected/*. This is why third.InjectToggleableScripts injects it.

Should I move stuff like the Alert function into fourth, then? I intended for most of the stuff in third to be used across injected scripts and internal scripts, but if there has to be some function migration between the two I'm fine with that. I might be misunderstanding you, but I feel like keeping third to the internal scripts in js/* kind of defeats the purpose I had initially created it for, but it's really not that big of a deal if that is the case.

aprzn123 commented 1 year ago

I think you might be underestimating what can be done without injection. (e.g. the pronoun display doesn't inject any scripts to work, it just runs directly from js/forum.js). What sort of situations do you expect Alert to be used in?

roxwize commented 1 year ago

Any. It's a general function.

aprzn123 commented 1 year ago

Might be the kind of thing to put in both third and fourth then. The main motivation for fourth was to simplify the API (especially wrt Sandvich's last PR, to abstract over the differing implementations on different pages, but also wrt accessing it from the injected scripts and preventing injected scripts from trying to inject again), but it might lead to a bunch of code duplication between the two. Perhaps we can set up a system to add certain methods to both third and fourth? (tempted to call it seventhHalf, but that's a bad idea lol)

roxwize commented 1 year ago

I'm sorry I haven't done much recently. Burn-out, like I said.

roxwize commented 1 year ago

I'll figure out how to resolve the conflict between third and fourth at some point. I think it might be better to flesh out their individual applications first and, if necessary, add a fifth module. But at that point, calling the global function modules by numbers would be impractical. It'd be better to just give them regular names (like global) past that point.