Initial event api, license headers #9

i509VCB · 2021-07-07T03:56:02Z

The first task here is the new event api for using with QSL. It is quite similar to the api fabric has minus some slight semantic differences and names.

one thing needs to be done before I merge this

Setup license headers

TheGlitch76

I don't think that copy-pasting Fabric classes is the right way to do this at all. It was not sustainable for Patchwork and I don't want to support that approach here.

Maintaining a fork of Fabric API with the implementation modified to wrap around QSL is infinitely more maintainable because we can merge upstream and keep track of when they change things.

We're not assuming that anything in QSL is considered stable, so it is OK to create Fabric's APIs again almost verbatim (name style changing as needed) until we determine what changes we want.

i509VCB · 2021-07-07T15:42:48Z

Hmm glitch that seems more reasonable tbf. However I'd want a few conditions:

Our backend reimplementation of fabric api MUST depend on a fixed version of QSL. Reason being I'll need to expose some internal compat bridges.

TheGlitch76 · 2021-07-07T15:47:58Z

Whatever you need--it's easier to change those kinds of things with experimentation, but it would be hard to lift FAPI out of QSL into a fork at a later date, so that's what's important to me.

i509VCB · 2021-07-13T05:24:38Z

Ready for review. Grammar checking especially.

Note the api is not final yet.

library/core/qsl-base/src/main/java/org/quiltmc/qsl/api/event/Event.java

LambdAurora · 2021-07-13T08:41:07Z

library/core/qsl-base/src/main/java/org/quiltmc/qsl/api/event/ParameterInvokingEvent.java

+@Documented // Documentation
+@Retention(RetentionPolicy.CLASS) // For static analysis
+@Target({ ElementType.FIELD, ElementType.METHOD })
+public @interface ParameterInvokingEvent {


That annotation name is still super obscure, at least the doc is clear.

Do you have a better idea?

Sadly, no.
I don't think this can be named in a clear way anyway, and this is the best name so far.

Writing the documentation is probably the best way to communicate what the annotation does.

ParameterImplementedEvent would be more clear to me, but at this point the documentation is good enough at describing its function that the name probably doesn't matter 😄

Ah the age old naming question.

We could go for a more Gradle-like naming scheme with something like InvokesParameterAsCallback for a name.

Technici4n · 2021-07-13T09:25:16Z

I'm not sure why you're randomly changing some names (besides ParameterInvokingEvent which is a fine rename). implementation for example is really confusing. And fallback instead of empty is worse as well imo.

library/core/qsl-base/src/main/java/org/quiltmc/qsl/api/event/Event.java

i509VCB · 2021-07-13T13:58:44Z

Fallback vs empty I'd be fine changing with some discussion.

Implementation was an intentional choice since I have interpreted an Event as a holder object for a generated implementation of T. In the case of an array event, T is implemented via multiple callbacks that implement T.

Haven-King · 2021-07-13T16:15:29Z

library/core/qsl-base/src/main/java/org/quiltmc/qsl/api/event/Event.java

+	 *
+	 * @param callback the callback.
+	 */
+	public abstract void register(T callback);


If we wanted to add callback IDs and dependencies, now would be the time to do it. The signature of register would then be something like

/** * Register a callback to the event. * * @param id the identifier of this listener * @param callback the callback * @param after an array of identifiers this callback needs to be run after, if they are present */ public abstract void register(Identifier id, T callback, Identifier... after);

This can fairly easily be implemented in a way that does not impact invocation-time performance. It does not solve all possible problems that might arise from invoker order, but it does offer one more (relatively pain free) mechanism to mod developers.

It's worth noting that for the purpose of Fabric mod compatibility, any implementation needs to be capable of handling cases where id is null, invoking it at any time.

before is necessary as well.

before is necessary as well.

That complicates things slightly as we can no longer just use varargs, but would be fine with some overrides (and probably better method names 😛).

/** * Register a callback to the event. * * @param id the identifier of this listener * @param callback the callback */ public void register(Identifier id, T callback) { this.register(id, callback, new Identifier[0], new Identifier[0]); } /** * Register a callback to the event. * * @param id the identifier of this listener * @param callback the callback * @param before an array of identifiers this callback needs to be run before, if they are present */ public void registerBefore(Identifier id, T callback, Identifier... before) { this.register(id, callback, before, new Identifier[0]); } /** * Register a callback to the event. * * @param id the identifier of this listener * @param callback the callback * @param after an array of identifiers this callback needs to be run after, if they are present */ public void registerAfter(Identifier id, T callback, Identifier... after) { this.register(id, callback, new Identifier[0], after); } /** * Register a callback to the event. * * @param id the identifier of this listener * @param callback the callback * @param before an array of identifiers this callback needs to be run before, if they are present * @param after an array of identifiers this callback needs to be run after, if they are present */ public abstract void register(Identifier id, T callback, Identifier[] before, Identifier[] after);

This seems very workable!

Though, shouldn't those T[]/T... arrays be Identifier[]/Identifier..., according to the javadocs?

This seems very workable!

Though, shouldn't those T[]/T... arrays be Identifier[]/Identifier..., according to the javadocs?

Oops, yes, I knew I missed something.

I know that, I was pointing out the incorrect typing on the varargs/arrays.

Not fully convinced on this. Do not the API is NOT final yet

Not convinced either, as Lambda explained on discord it makes every ID a rigid part of a mod's API.

Screenshot of a compromise I might accept for int priorities given some surrounding conditions:

https://media.discordapp.net/attachments/522124386128494599/865243382997844008/Screenshot_20210715-094323_Discord.jpg

Haven seems to be on the other side of the coin on identifiers, I do note that identifiers have some advantages but some disadvantages that integers handle properly

changing priority ordering is an intentional act, you must have something to be ordered and know what you are ordered before.

no reordering is done given the absence of dependencies

There is now an issue for this chain: #14

Co-authored-by: LambdAurora <aurora42lambda@gmail.com>

library/core/qsl-base/src/main/java/org/quiltmc/qsl/api/event/Event.java

i509VCB · 2021-07-14T23:31:28Z

@Technici4n returned to empty naming on the special createArrayEvent

Technici4n

I think this is a good occasion to remove useless API complexity. Maybe ArrayEvent should even be inlined into Event directly.

library/core/qsl-base/src/main/java/org/quiltmc/qsl/api/event/EventFactory.java

library/core/qsl-base/src/main/java/org/quiltmc/qsl/api/event/Event.java

Technici4n · 2021-07-15T07:51:56Z

library/core/qsl-base/src/main/java/org/quiltmc/qsl/api/event/Event.java

+	 *
+	 * @param callback the callback.
+	 */
+	public abstract void register(T callback);


Not convinced either, as Lambda explained on discord it makes every ID a rigid part of a mod's API.

TheGlitch76

Mostly just some documentation clunk that needs fixed, and this shouldn't be merged until we get the vote on #10 tomorrow or so.

library/core/qsl-base/src/main/java/org/quiltmc/qsl/api/event/ArrayEvent.java

library/core/qsl-base/src/main/java/org/quiltmc/qsl/api/event/ParameterInvokingEvent.java

TheGlitch76 · 2021-07-20T01:34:49Z

I'm still unsure if we should resolve the debate of int/string/no priority before merging this PR.
We can swap it out at any time before API is stabilized, of course, but that doesn't mean we should wait until the last moment.

Co-authored-by: Glitch <glitch.g3431@gmail.com>

Leo40Git · 2021-07-20T20:46:47Z

Heads up: decided package structure is org.quiltmc.qsl.$module.api/impl!

i509VCB · 2021-07-22T22:32:03Z

I think we are done with this.

i509VCB added 2 commits July 2, 2021 23:31

Stash current progress

32ed353

Implement Fabric event api

d19bc3a

TheGlitch76 requested changes Jul 7, 2021

View reviewed changes

i509VCB added 8 commits July 8, 2021 16:21

Make sure builds target java 16

bef9251

Add ParameterInvokingEvent annotation

eca9388

invokerFactory -> implementation to match the docs of Event

51c4d6c

EventFactory docs

9c74805

Explain event execution

39fc480

little typo

6b8870f

yeet

fee993c

don't care about what fabric does, they can run a parallel impl

c87274e

i509VCB marked this pull request as ready for review July 13, 2021 05:24

i509VCB requested review from TheGlitch76 and a team July 13, 2021 05:24

LambdAurora reviewed Jul 13, 2021

View reviewed changes

Technici4n reviewed Jul 13, 2021

View reviewed changes

library/core/qsl-base/src/main/java/org/quiltmc/qsl/api/event/Event.java Outdated Show resolved Hide resolved

Haven-King reviewed Jul 13, 2021

View reviewed changes

Apply suggestions from code review

48df8ec

Co-authored-by: LambdAurora <aurora42lambda@gmail.com>

Pyrofab reviewed Jul 14, 2021

View reviewed changes

library/core/qsl-base/src/main/java/org/quiltmc/qsl/api/event/Event.java Outdated Show resolved Hide resolved

use a dedicated functional interface for the example

1ef864b

Return to empty

b218574

i509VCB changed the title ~~Implement Events and reimplement Fabric's Event api~~ Initial event api Jul 15, 2021

Technici4n suggested changes Jul 15, 2021

View reviewed changes

Flatten Event down to a final class, ArrayEvent

fd9916d

TheGlitch76 requested changes Jul 20, 2021

View reviewed changes

i509VCB and others added 3 commits July 20, 2021 01:54

Apply suggestions from code review

6478e92

Co-authored-by: Glitch <glitch.g3431@gmail.com>

Use @code so we don't need entity codes

1be1a56

typo and reflow

ba537f5

i509VCB added 2 commits July 22, 2021 17:08

Match package name conventions decided on

c08dbba

loicenses

e675ad7

i509VCB changed the title ~~Initial event api~~ Initial event api, license headers Jul 22, 2021

source and target compat don't matter since it's java 16

fac8102

LambdAurora approved these changes Jul 22, 2021

View reviewed changes

LambdAurora requested a review from a team July 22, 2021 22:38

i509VCB merged commit 1f6a7dc into QuiltMC:1.17 Jul 22, 2021

This was referenced Jul 22, 2021

I made a fatal mistake: event api, license headers #15

Merged

Enhancing ParameterInvokingEvent #16

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial event api, license headers #9

Initial event api, license headers #9

i509VCB commented Jul 7, 2021 •

edited

Loading

TheGlitch76 left a comment

i509VCB commented Jul 7, 2021

TheGlitch76 commented Jul 7, 2021 •

edited

Loading

i509VCB commented Jul 13, 2021

LambdAurora Jul 13, 2021

Technici4n Jul 13, 2021

LambdAurora Jul 13, 2021

i509VCB Jul 13, 2021

Haven-King Jul 13, 2021

i509VCB Jul 14, 2021

Technici4n commented Jul 13, 2021

i509VCB commented Jul 13, 2021

Haven-King Jul 13, 2021 •

edited

Loading

Technici4n Jul 13, 2021

Haven-King Jul 13, 2021 •

edited

Loading

Leo40Git Jul 13, 2021

Haven-King Jul 13, 2021

Leo40Git Jul 13, 2021

i509VCB Jul 14, 2021

Technici4n Jul 15, 2021

i509VCB Jul 15, 2021

i509VCB Jul 22, 2021

i509VCB commented Jul 14, 2021

Technici4n left a comment

Technici4n Jul 15, 2021

TheGlitch76 left a comment

TheGlitch76 commented Jul 20, 2021

Leo40Git commented Jul 20, 2021

i509VCB commented Jul 22, 2021

Initial event api, license headers #9

Initial event api, license headers #9

Conversation

i509VCB commented Jul 7, 2021 • edited Loading

TheGlitch76 left a comment

Choose a reason for hiding this comment

i509VCB commented Jul 7, 2021

TheGlitch76 commented Jul 7, 2021 • edited Loading

i509VCB commented Jul 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Technici4n commented Jul 13, 2021

i509VCB commented Jul 13, 2021

Haven-King Jul 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Haven-King Jul 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

i509VCB commented Jul 14, 2021

Technici4n left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheGlitch76 left a comment

Choose a reason for hiding this comment

TheGlitch76 commented Jul 20, 2021

Leo40Git commented Jul 20, 2021

i509VCB commented Jul 22, 2021

i509VCB commented Jul 7, 2021 •

edited

Loading

TheGlitch76 commented Jul 7, 2021 •

edited

Loading

Haven-King Jul 13, 2021 •

edited

Loading

Haven-King Jul 13, 2021 •

edited

Loading