raidboss: refactor trigger loading #5047

trim21 · 2022-11-01T23:18:37Z

this contains the change of #5045 and #5038, please review and merge them first

This PR seprated triggers loading into 3 seprated steps, loading, localizing and overriding for better maintenance

ui/raidboss/raidboss_options.ts

ui/raidboss/popup-text.ts

quisquous · 2022-11-02T18:51:57Z

ui/raidboss/popup-text.ts

-//
-// JavaScript dictionaries are *almost* ordered automatically as we would want,
-// but want to handle missing ids and integer ids (you shouldn't, but just in case).
-class OrderedTriggerList {


What was wrong with OrderedTriggerList?

Nothing, just it mix-up the localization step and override step, and now they are separated.

I guess I don't really understand. You could replace ProcessedTrigger with LocalizedTrigger and this code would still work. Mostly I just know this existing code is working, and so I'd rather not replace it with something new, unless there's a reason to rewrite it.

In particular, I think it's a little confusing from a user output error to have the override console messages come in the opposite order that they are overridden. If you have A B C triggers in order, this will print out C overrides B, and B overrides A, but in reality the load order is A B C.

ui/raidboss/popup-text.ts

ui/raidboss/raidboss_config.ts

Co-authored-by: Adrienne Walker <[email protected]>

…ading

ui/raidboss/popup-text.ts

trim21 · 2022-11-03T01:07:22Z

Should we add legacy field like regexDe to loose trigger?

There is a deprecated zoneRegex field, I think also need to add regexDe/netRegexDe fields

quisquous · 2022-11-03T19:03:20Z

ui/raidboss/popup-text.ts

-//
-// JavaScript dictionaries are *almost* ordered automatically as we would want,
-// but want to handle missing ids and integer ids (you shouldn't, but just in case).
-class OrderedTriggerList {


I guess I don't really understand. You could replace ProcessedTrigger with LocalizedTrigger and this code would still work. Mostly I just know this existing code is working, and so I'd rather not replace it with something new, unless there's a reason to rewrite it.

quisquous · 2022-11-03T19:06:17Z

ui/raidboss/raidboss_config.ts

@@ -1453,20 +1464,23 @@ const userFileHandler: UserFileCallback = (
  if (!baseOptions.Triggers)
    return;

+  baseOptions.Triggers ??= [];
+  baseOptions.LoadedTriggers ??= [];


I still think LoadedTriggers is an implementation detail and it doesn't make sense for it to be part of Options. It's not something set by a user or the config.

If filename is part of LooseTriggerSet, then Triggers can just be updated in place as it used to be, right? That's what I was trying to suggest before.

I'm trying not to modified object in place 🤔

In general, I think "don't modify things in place" is a good approach, but usually that means there needs to be a clear data pipeline or data should be in temporary scoped variables. I think passing things through Options that shouldn't live in Options is strictly worse. If anything, you're also now modifying Options in place, which seems like the same issue.

(Also, this is already being modified in place on line 1336.)

Since there's already a lot going on here, what do you think about an approach of: (1) in this PR, keep modifying things in place / keep Options as-is without new members (2) follow up with a different PR to change how triggers are loaded here and maybe return different things from this function, make it better?

quisquous · 2022-11-03T19:08:12Z

ui/raidboss/raidboss_config.ts

+  return [set.timeline, files[timelineFile]];
+};
+
+const flattenTimelinePlace = (


Can this function delete set.timelineFile then if you've removed it from the other function, since it's overriding the timeline?

quisquous · 2022-11-03T19:08:57Z

ui/raidboss/popup-text.ts

+  timelineTriggers?: LooseTimelineTrigger[];
+  triggers?: LooseTrigger[];


Suggested change

timelineTriggers?: LooseTimelineTrigger[];

triggers?: LooseTrigger[];

Aren't these already defined in LooseTriggerSet? I'm not sure I understand why they need to be re-listed here.

LooseTriggerSet now is triggers?: (LooseTrigger & LegacyTrigger)[];

and

export type LegacyTrigger = { regexEn?: RegExp; regexDe?: RegExp; regexFr?: RegExp; regexCn?: RegExp; regexJa?: RegExp; regexKo?: RegExp; // @deprecated netRegexEn?: RegExp; netRegexFr?: RegExp; netRegexDe?: RegExp; netRegexCn?: RegExp; netRegexJa?: RegExp; netRegexKo?: RegExp; };

quisquous · 2022-11-03T19:14:32Z

Should we add legacy field like regexDe to loose trigger?

Sure, sounds good!

quisquous · 2022-11-05T01:01:18Z

resources/languages.ts

+/**
+ * @deprecated remove this after we don't support `netRegexCn` style triggers
+ */
+export type LegacyLangSuffix = 'En' | 'De' | 'Fr' | 'Ja' | 'Cn' | 'Ko';


Where does this get used?

regex${lang as LegacyLangSuffix} netRegex${lang as LegacyLangSuffix}

Sorry, I don't see what you're referring to. Which file is that in?

it's used to get legacy regexDe/netRegexDe property in popup-text.ts

$ git grep LegacyLangSuffix resources/languages.ts:export type LegacyLangSuffix = 'En' | 'De' | 'Fr' | 'Ja' | 'Cn ' | 'Ko';

Sorry for the confusion, I just don't see what you mean. Did you mean to add these changes and they didn't save/commit?

that's strange, I remembered I used it in popup-test.ts, maybe I lost it by mistake.

quisquous · 2022-11-05T01:02:35Z

types/trigger.d.ts

+/**
+ *  @deprecated
+ */
+export type LegacyTrigger = {


I think this could be combined into LooseTrigger. LooseTrigger is the "user trigger, that could be missing fields, and could have old deprecated fields in it" type. It's the same reason that LooseTriggerSet has zoneRegex in it.

ui/raidboss/raidboss_options.ts

quisquous · 2022-11-05T01:20:48Z

ui/raidboss/popup-text.ts

-//
-// JavaScript dictionaries are *almost* ordered automatically as we would want,
-// but want to handle missing ids and integer ids (you shouldn't, but just in case).
-class OrderedTriggerList {


In particular, I think it's a little confusing from a user output error to have the override console messages come in the opposite order that they are overridden. If you have A B C triggers in order, this will print out C overrides B, and B overrides A, but in reality the load order is A B C.

Co-authored-by: Adrienne Walker <[email protected]>

github-actions bot added config raidboss test eureka plugin resources and removed resources eureka plugin labels Nov 1, 2022

trim21 added 3 commits November 2, 2022 07:26

merge quisquous#5038

033afa1

merge quisquous#5045

c89701e

real change of this PR

eec49c3

trim21 force-pushed the refactor-trigger-loading branch from 4552be1 to eec49c3 Compare November 1, 2022 23:28

trim21 added 3 commits November 2, 2022 08:10

handle trigger override

7cac6c5

use map to handle added trigger

3915822

remove dead code

c448a95

trim21 marked this pull request as ready for review November 2, 2022 00:30

trim21 marked this pull request as draft November 2, 2022 05:36

quisquous reviewed Nov 2, 2022

View reviewed changes

trim21 and others added 3 commits November 3, 2022 03:41

Update ui/raidboss/popup-text.ts

06aafa8

Co-authored-by: Adrienne Walker <[email protected]>

Update ui/raidboss/popup-text.ts

9453c34

Co-authored-by: Adrienne Walker <[email protected]>

Merge remote-tracking branch 'upstream/main' into refactor-trigger-lo…

54d6bf1

…ading

github-actions bot removed the test label Nov 2, 2022

trim21 added 5 commits November 3, 2022 04:08

some code review

4fdec62

fix LooseTimelineTrigger

849c070

fix undefined id

a1259ca

fix always true case

263476d

fix always true case

a85117b

trim21 marked this pull request as ready for review November 2, 2022 20:32

read from user set filename first

0d52a72

avoid override

e9fb6e9

quisquous reviewed Nov 3, 2022

View reviewed changes

ui/raidboss/popup-text.ts Show resolved Hide resolved

wip

2fec0f4

quisquous reviewed Nov 3, 2022

View reviewed changes

trim21 added 2 commits November 5, 2022 00:37

wip

3ade816

code review

e6f9d8f

github-actions bot added the resources label Nov 4, 2022

quisquous reviewed Nov 5, 2022

View reviewed changes

Update ui/raidboss/raidboss_options.ts

ecbffef

Co-authored-by: Adrienne Walker <[email protected]>

trim21 mentioned this pull request Dec 31, 2022

disabled trigger require regex or netRegex #4965

Closed

trim21 closed this May 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

raidboss: refactor trigger loading #5047

raidboss: refactor trigger loading #5047

trim21 commented Nov 1, 2022 •

edited

Loading

quisquous Nov 2, 2022

trim21 Nov 2, 2022 •

edited

Loading

quisquous Nov 3, 2022

quisquous Nov 5, 2022

trim21 commented Nov 3, 2022 •

edited

Loading

quisquous Nov 3, 2022

quisquous Nov 3, 2022

trim21 Nov 4, 2022 •

edited

Loading

quisquous Nov 4, 2022 •

edited

Loading

quisquous Nov 3, 2022

quisquous Nov 3, 2022

trim21 Nov 4, 2022

quisquous commented Nov 3, 2022

quisquous Nov 5, 2022

trim21 Nov 5, 2022

quisquous Nov 5, 2022

trim21 Nov 5, 2022

quisquous Nov 5, 2022

quisquous Nov 5, 2022

trim21 Nov 5, 2022

quisquous Nov 5, 2022

quisquous Nov 5, 2022

		timelineTriggers?: LooseTimelineTrigger[];
		triggers?: LooseTrigger[];

raidboss: refactor trigger loading #5047

raidboss: refactor trigger loading #5047

Conversation

trim21 commented Nov 1, 2022 • edited Loading

Choose a reason for hiding this comment

trim21 Nov 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trim21 commented Nov 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trim21 Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

quisquous Nov 4, 2022 • 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

quisquous commented Nov 3, 2022

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

Choose a reason for hiding this comment

trim21 commented Nov 1, 2022 •

edited

Loading

trim21 Nov 2, 2022 •

edited

Loading

trim21 commented Nov 3, 2022 •

edited

Loading

trim21 Nov 4, 2022 •

edited

Loading

quisquous Nov 4, 2022 •

edited

Loading