WIP: make sparkle class (?) #38

2026-05-18T19:45:48-05:00

codingisfun2831t commented

2026-05-18 19:45:48 -05:00

(Migrated from github.com)

WIP

Some mods have issues with these changes (I did change some API naming. However, we are still in v0.x.x so I think its fine), so would I be able to make a PR to the addons after this?

WIP Some mods have issues with these changes (I did change some API naming. However, we are still in v0.x.x so I think its fine), so would I be able to make a PR to the addons after this?

👀 1

PPPDUD commented

2026-05-19 07:19:51 -05:00

(Migrated from github.com)

Upon a first review, it looks excellent. I would advise that you open a draft PR in SparkleAddons too and I'll merge both at some point soon.

In the future, you shouldn't change comments just because they say Crackle, but it's harmless so I'll let it slide this time.

Keep up the good work!

Upon a first review, it looks excellent. I would advise that you open a draft PR in SparkleAddons too and I'll merge both at some point soon. In the future, you shouldn't change comments just because they say Crackle, but it's harmless so I'll let it slide this time. Keep up the good work!

e016 commented

2026-05-19 09:44:36 -05:00

(Migrated from github.com)

I'm not sure about changing the api.storage object to seperate functions; but everything else looks good!

codingisfun2831t commented

2026-05-19 17:12:51 -05:00

(Migrated from github.com)

Well, doesn't exactly make sense (can't really do it properly in a class)

One thing I want to ask: Who make the storage.set/get functions? localStorage.getItem/setItem is right there.

Well, doesn't exactly make sense (can't really do it properly in a class) One thing I want to ask: Who make the storage.set/get functions? localStorage.getItem/setItem is right there.

PPPDUD commented

2026-05-19 17:18:11 -05:00

(Migrated from github.com)

Well, doesn't exactly make sense (can't really do it properly in a class)

One thing I want to ask: Who make the storage.set/get functions? localStorage.getItem/setItem is right there.

I think that the idea is that addons don't need to remove stuff from localStorage manually upon removal, because Sparkle does that automatically.

> Well, doesn't exactly make sense (can't really do it properly in a class) > > One thing I want to ask: Who make the storage.set/get functions? localStorage.getItem/setItem is right there. I think that the idea is that addons don't need to remove stuff from localStorage manually upon removal, because Sparkle does that automatically.

e016 commented

2026-05-19 17:20:54 -05:00

(Migrated from github.com)

Well, doesn't exactly make sense (can't really do it properly in a class)

One thing I want to ask: Who make the storage.set/get functions? localStorage.getItem/setItem is right there.

I did make the storage.get/set!

I think that the idea is that addons don't need to remove stuff from localStorage manually upon removal, because Sparkle does that automatically.

Exactly.

> Well, doesn't exactly make sense (can't really do it properly in a class) > > One thing I want to ask: Who make the storage.set/get functions? localStorage.getItem/setItem is right there. I did make the storage.get/set! > I think that the idea is that addons don't need to remove stuff from localStorage manually upon removal, because Sparkle does that automatically. _Exactly._

PPPDUD commented

2026-05-19 17:22:05 -05:00

(Migrated from github.com)

Well, doesn't exactly make sense (can't really do it properly in a class)
One thing I want to ask: Who make the storage.set/get functions? localStorage.getItem/setItem is right there.

I did make the storage.get/set!

I think they meant to type "why" and not "who".

> > Well, doesn't exactly make sense (can't really do it properly in a class) > > One thing I want to ask: Who make the storage.set/get functions? localStorage.getItem/setItem is right there. > > I did make the storage.get/set! > I think they meant to type "why" and not "who".

codingisfun2831t commented

2026-05-19 17:30:13 -05:00

(Migrated from github.com)

No, I'm talking about the one in crackle. Not API, that makes sense and is good, but the one in crackle that just calls those values.

No, I'm talking about the one in __crackle__. Not API, that makes sense and is good, but the one in __crackle__ that just calls those values.

codingisfun2831t commented

2026-05-19 17:31:18 -05:00

(Migrated from github.com)

Oh also the wrapFunction logic is kind of over the place, and theres a bug there. So uh oh :~(

e016 commented

2026-05-20 12:58:17 -05:00

(Migrated from github.com)

Oh also the wrapFunction logic is kind of over the place, and theres a bug there. So uh oh :~(
Yeah, I fixed it in my important parameter PR.

> Oh also the wrapFunction logic is kind of over the place, and theres a bug there. So uh oh :~( Yeah, I fixed it in my important parameter PR.

codingisfun2831t commented

2026-05-26 16:24:20 -05:00

(Migrated from github.com)

So the merge conflicts are too much.... Can anyone make a PR to the branch to fix the merge conflicts? I'm not the best one to do that (visual studio code keeps jumping around..)

PPPDUD commented

2026-05-27 14:35:41 -05:00

(Migrated from github.com)

So the merge conflicts are too much.... Can anyone make a PR to the branch to fix the merge conflicts? I'm not the best one to do that (visual studio code keeps jumping around..)

Busy with some other stuff at the moment, and I think that @e016 is too (@e016 correct me if I'm wrong here). Maybe @Bubgamer07 would be willing to help out?

> So the merge conflicts are too much.... Can anyone make a PR to the branch to fix the merge conflicts? I'm not the best one to do that (visual studio code keeps jumping around..) Busy with some other stuff at the moment, and I think that @e016 is too (@e016 correct me if I'm wrong here). Maybe @Bubgamer07 would be willing to help out?

Bubgamer07 commented

2026-05-27 17:07:47 -05:00

(Migrated from github.com)

So the merge conflicts are too much.... Can anyone make a PR to the branch to fix the merge conflicts? I'm not the best one to do that (visual studio code keeps jumping around..)

Busy with some other stuff at the moment, and I think that @e016 is too (@e016 correct me if I'm wrong here). Maybe @Bubgamer07 would be willing to help out?

sure, I'll sort it out when I get home.

> > So the merge conflicts are too much.... Can anyone make a PR to the branch to fix the merge conflicts? I'm not the best one to do that (visual studio code keeps jumping around..) > > Busy with some other stuff at the moment, and I think that @e016 is too (@e016 correct me if I'm wrong here). Maybe @Bubgamer07 would be willing to help out? sure, I'll sort it out when I get home.

👍 1

PPPDUD commented

2026-05-27 17:17:41 -05:00

(Migrated from github.com)

So the merge conflicts are too much.... Can anyone make a PR to the branch to fix the merge conflicts? I'm not the best one to do that (visual studio code keeps jumping around..)

Busy with some other stuff at the moment, and I think that @e016 is too (@e016 correct me if I'm wrong here). Maybe @Bubgamer07 would be willing to help out?

sure, I'll sort it out when I get home.

@Bubgamer07 Stuff like this is why I'm glad to have you on the team. You're awesome.

> > > So the merge conflicts are too much.... Can anyone make a PR to the branch to fix the merge conflicts? I'm not the best one to do that (visual studio code keeps jumping around..) > > > > > > Busy with some other stuff at the moment, and I think that @e016 is too (@e016 correct me if I'm wrong here). Maybe @Bubgamer07 would be willing to help out? > > sure, I'll sort it out when I get home. @Bubgamer07 Stuff like this is why I'm glad to have you on the team. You're awesome.

Bubgamer07 commented

2026-05-28 09:17:51 -05:00

(Migrated from github.com)

@PPPDUD @codingisfun2831t it is done (Hopefully if I did it right)

PPPDUD commented

2026-05-28 09:19:36 -05:00

(Migrated from github.com)

@PPPDUD @codingisfun2831t it is done (Hopefully if I did it right)

Looks awesome! Thanks for the help.

> @PPPDUD @codingisfun2831t it is done (Hopefully if I did it right) Looks awesome! Thanks for the help.

Bubgamer07 commented

2026-05-28 09:40:23 -05:00

(Migrated from github.com)

I know I probably messed something up though :p

codingisfun2831t commented

2026-05-28 14:03:56 -05:00

(Migrated from github.com)

Think its ready for basic review & merging. But wait until I fix up the Sparkle addons (especially for the new wrapFunction)

PPPDUD (Migrated from github.com) reviewed

2026-05-29 11:28:53 -05:00

PPPDUD (Migrated from github.com) left a comment

Upon a quick review, I don't see any huge issues besides the copyright date. I still need to fully test it out before approving though.

ext.js

					
				@ -0,0 +1,26 @@

				/*

PPPDUD commented

2026-05-29 11:32:40 -05:00

(Migrated from github.com)

@codingisfun2831t Renaming index.js to sparkle.js broke package.sh, but I fixed it in commit 910d40a.

@codingisfun2831t Renaming `index.js` to `sparkle.js` broke `package.sh`, but I fixed it in commit 910d40a.

PPPDUD (Migrated from github.com) requested changes

2026-05-29 11:42:32 -05:00

PPPDUD (Migrated from github.com) left a comment

Generally, it looks like you've implemented this PR pretty well. ~~but I don't like how addons can't access Sparkle's internal state anymore. Perhaps we should have a this.api._sparkleInternalState object for addons that require messing with Sparkle's internals?~~ Edit: It seems like this.sparkle does what I've described here.

Generally, it looks like you've implemented this PR pretty well. ~but I don't like how addons can't access Sparkle's internal state anymore. Perhaps we should have a `this.api._sparkleInternalState` object for addons that require messing with Sparkle's internals?~ Edit: It seems like `this.sparkle` does what I've described here.

PPPDUD (Migrated from github.com) reviewed

2026-05-29 11:55:42 -05:00

sparkle.js

					
				@ -0,0 +958,4 @@

				                this.fixOptionsLayout();

				                break;

				        }

				    }

This line is broken because window.__crackle__ no longer exists.

This line is broken because `window.__crackle__` no longer exists.

PPPDUD (Migrated from github.com) requested changes

2026-05-29 11:56:47 -05:00

PPPDUD (Migrated from github.com) left a comment

Also, loading addons from code is broken and just opens another input box window when the OK button is pressed.

PPPDUD commented

2026-05-29 11:57:54 -05:00

(Migrated from github.com)

I'm turning this back into a draft because it has way more issues than I initially expected. @codingisfun2831t Please mark this as ready for review when you've fixed the bugs that I've reported.

PPPDUD commented

2026-05-29 12:14:41 -05:00

(Migrated from github.com)

@Bubgamer07 I think that I accidentally caused conflicts again in #49; would you please take a look and see if you can fix it?

Bubgamer07 commented

2026-05-29 12:21:43 -05:00

(Migrated from github.com)

@Bubgamer07 I think that I accidentally caused conflicts again in #49; would you please take a look and see if you can fix it?

When I get home again, yes

> @Bubgamer07 I think that I accidentally caused conflicts again in #49; would you please take a look and see if you can fix it? When I get home again, yes

👍 1

codingisfun2831t commented

2026-05-30 10:37:46 -05:00

(Migrated from github.com)

Fixed all the issues. However, I still want to cleanup the codebase a tad, but I'll make that a seperate PR. I'll work on fixing the addons now.

codingisfun2831t commented

2026-05-30 10:43:25 -05:00

(Migrated from github.com)

I think I should probably close this and make a new cleanup PR that re does the Sparkle class (lots of random stuff thats broken via merge conflicts) and I'll also cleanup some other stuff. So can everyone stop doing development while I'm doing that (should be able to do it today) so no merge conflicts?

PPPDUD commented

2026-05-30 10:52:49 -05:00

(Migrated from github.com)

I think I should probably close this and make a new cleanup PR that re does the Sparkle class (lots of random stuff thats broken via merge conflicts) and I'll also cleanup some other stuff. So can everyone stop doing development while I'm doing that (should be able to do it today) so no merge conflicts?

Will do.

> I think I should probably close this and make a new cleanup PR that re does the Sparkle class (lots of random stuff thats broken via merge conflicts) and I'll also cleanup some other stuff. So can everyone stop doing development while I'm doing that (should be able to do it today) so no merge conflicts? Will do.

codingisfun2831t commented

2026-05-30 11:56:46 -05:00

(Migrated from github.com)

Update: I give up on this