Proposal: "auto" cleaning cutin in script

[guilherme-gm]

Posting here to see if I can get some feedback before implementing it

Is your feature request related to a problem? Please describe.
This is a proposal to try to improve cutin cleanup in scripts. Today, when using cutin in a dialog, we must remember to remove them at the end of the dialog, on every close (unless, in the very very very few cases that we might want to keep it, we know some scripts in the wild abuse that to make new stuff).

For example:

	cutin("fly_felrock", 2);
	mes("[Tarlock]");
	mes("I am Tarlock,");
	close2();
	cutin("", 255);
	end;

Today, we have about 700 occurrences of that pattern (in a quick RegExp find through herc folder).

My proposal is to provide a way to:

1. Simplify this clean up (we need 3 lines for that!)
2. If possible, make it harder to forget a clean up

I think someone at rAthena also noticed that, as they currently have close3 command, which does basically the close2 + cutin + end in a single command (more on that in Solutions)

Solutions

I thought about 3 possible solutions, I will write them below and the pros/cons I could think of.

(My preferred) Solution 1. dlgcutin script command

In this solution, we would introduce a new command: dlgcutin("<file name>", <position>)

This command would be an alias to cutin, but when used would set a flag in the script execution telling it should clear the cutin up after a close. Rewriting the previous example:

	dlgcutin("fly_felrock", 2);
	mes("[Tarlock]");
	mes("I am Tarlock,");
	close(); //<== close2(); could also have been used if we had something to do here, e.g.: warp

If cutin were to be used after dlgcutin, the flag would be cleared up -- this would allow scripters freedom to switch between auto closing or not based on their needs.

Pros:

1. We make it explicit that this cutin is related to the dialog and should be closed (or should not) automatically
2. close and close2 can be used as always for their normal usage, with no worries about cutins
3. No breaking changes

Cons:

1. Initial conversion is a bit more cumbersome (can't simply do a RegExp and call it a day)
2. Conversion between rA and Herc now has one different command to take care of

Solution 2. Extending cutin

Going back to the cutin syntax:

cutin("<filename>", <position>)

where position:
	0 - bottom left corner
	1 - bottom middle
	2 - bottom right corner
	3 - middle of screen in a movable window with an empty title bar
	4 - middle of screen without the window header, but still movable
  255 - remove

For this solution, my proposal is to either:

1. Add a 3rd parameter "auto close" (or not auto close)
2. Add a bit flag to the position (like position | 512 means auto close -- or not auto close)

One thing that would need to be discussed here is whether the default would be to auto close, as is probably used in 99% of the cases, but would introduce a silent breaking change, or require us to explicitly say it should auto close -- and we will have to update every place with the 3rd parameter.

Rewriting the example from before:

	cutin("fly_felrock", 2, true);
	// or: cutin("fly_felrock", 2 | CUTIN_AUTOCLOSE);
	mes("[Tarlock]");
	mes("I am Tarlock,");
	close(); //<== close2(); could also have been used if we had something to do here, e.g.: warp

Pros:

1. Same command
2. close and close2 can be used as always for their normal usage, with no worries about cutins

Cons:

1. Somewhat more verbose, specially if we chose that we need to be explicit about auto-closing
2. Introduces a breaking change if we chose that auto-closing is implicit
3. Initial conversion is a bit more cumbersome (can't simply do a RegExp and call it a day)
4. Conversion between rA and Herc now has one different command to take care of

Solution 3. (My least preferred) close3 script command

This would copy how rAthena did it, a new command, close3() which would:

1. Show close button and set a flag in the script execution
2. Once player presses the close button, the cutin would be removed

It would be exactly like the close2 + cutin + end, but in a single command.

Example:

	cutin("fly_felrock", 2);
	mes("[Tarlock]");
	mes("I am Tarlock,");
	close3();

Pros:

1. No need to convert between emulators
2. Very straightforward to implement (a simple RegExp should be able to do it)

Cons:

1. If you need to do close2, you will have to clean up manually
2. The cutin clean up becomes "implicit" into the close command, while not bad, it is a detail to remember

[Streusel]

Assuming solution 2 is chosen, which seems like a decent way to go about it. Making parameter no. 3 (auto_close) an optional parameter that defaults to true if not provided would be great way to address this.

[guilherme-gm]

the issue with making it optional and defaulting to true is that we will break any custom script that actually expects it to stay open (like scripts that uses cutins to show some custom "UI")

[TioAkima]

the issue with making it optional and defaulting to true is that we will break any custom script that actually expects it to stay open (like scripts that uses cutins to show some custom "UI")

Yes, I have some custom "UI" that uses cutins and this solution could cause some problems. I would have to rewrite the code.

Well, solution 1 is actually good because it is a silent modification, there is no need to change old scripts or anything like that (besides allowing auto closing or not, according to your needs).

It's really the most interesting

[Streusel]

Yes, I have some custom "UI" that uses cutins and this solution could cause some problems. I would have to rewrite the code.

Wouldn't that simply be adding the 3rd parameter and setting it to false for you. In terms of updating the codebase I don't think it's a huge issue. It's not a breaking change but one that requires updates. That's normal and to be expected with time, no?

[guilherme-gm]

I do consider a breaking change anything that requires a code change. But I don't think we should be against breaking changes, but when needed, we should safely break (as in: blow up as early as possible)

If I have a script working fine today, and a few months from now download a fresh copy of Herc and put that script there, I would expect that it would either:

1. Work as before
2. Give me errors at load time (Breaking change that I know I have to fix)

As far as I know, Herc has historically always avoided as much as possible silently breaking something. (some refs below)

If we change cutin and default it to auto-closing, every script will silently adhere to that. So going back to the above example, it would start just fine, I would think it is fine, until a user runs into the issue and I need to figure out that cutin has a new behavior (after debugging the code, scratching my head because this has always worked fine for 20 years)

If we follow the pattern of not silently breaking something, we would need to either:

1. default to not auto close (so old scripts work as is)
2. or make the auto close parameter required (so everybody needs to update their script).

One of those 2 options would be fine, but I think they would be ergonomically worse.

Some examples of where the above happened:

• Update to getmobdrops command: getmobdrops refactor #3319 (comment)
  • skyleo requested to ensure users would get an error instead of silently failing (thus, someone using it would be required to review)
  • This was addressed by having a new required parameter -- a breaking change that "blows up as soon as you run"
• Changed the behavior of a clif-> function: Fix Soul Balls in newer clients #3291 (comment)
  • Haru requested to rename the function as the signature was the same (thus, someone modifying it would be required to review)
  • a breaking change that "blows up as soon as you compile"
• Update to getcalendartime command: Localtime/daylight savings enhancements and fixes #3224 (comment)
  • Haru ensured backward compatibility by making the optional parameter default to the old behavior
  • no breaking change

I could not find examples where we chose to silently break something (at least not intentionally)

[jasonch35]

I like Solution 2 better since it follows standard practice. Some script commands were combined into one that does the same job. Not really a fan of close3 as well, it’s not very clear and doesn’t seem related to cutins.