Introduces toggle effect & expression #4154

Olyno · 2021-07-03T15:05:39Z

Description

This pull request introduces the toggle effect and expression.

set {_var} to true
toggle {_var} # {_var} is now false

set {_var} to inverse of {_var} # {_var} is now true

Target Minecraft Versions: /
Requirements: /
Related Issues: /

Mwexim · 2021-07-03T18:13:50Z

I have the same question I had with Delta’s PR initially: why would you add a new ChangeMode value? Toggling a boolean is essentially equal to setting the boolean to its opposite value.

If anyone can give me one type that could use toggling except booleans, then go ahead, but in my opinion this should just be added as a separate effect and use the set-operation to perform the action.

TPGamesNL

Expression's javadocs for change and acceptChange should be updated

src/main/java/ch/njol/skript/expressions/ExprEventCancelled.java

src/main/java/ch/njol/skript/expressions/ExprGlowing.java

src/main/java/ch/njol/skript/expressions/ExprSaturation.java

src/main/java/ch/njol/skript/expressions/ExprWhitelist.java

src/main/java/ch/njol/skript/effects/EffChange.java

Co-authored-by: TPGamesNL <[email protected]>

…feature/toggleChanger

Olyno · 2021-07-03T20:40:18Z

why would you add a new ChangeMode value?

It's more intuitive and makes more sense than toggle the value manually. For example:

set flight mode to false # Bruh
toggle flight mode # This is good

Toggling a boolean is essentially equal to setting the boolean to its opposite value

Yes it is, but what's the most intuitive for users? Having a boolean "state" expression, or use a simple toggle?

in my opinion this should just be added as a separate effect and use the set-operation to perform the action

I did that in a first time, but try to reuse an effect in addons, this is just impossible or really tricky/hard to do. What we would like is a simple api for addons developers that anyone could use it easily. Adding a mode makes things easier than adding an effect, and trying to use this effect.
I would like add too a changer is already an effect. Wouldn't be better to group up all changer effects at the same place? 😃

Mwexim · 2021-07-03T21:41:05Z

You're understanding me wrong. What I'm saying is that the internal structure of this feature does not make sense to me.

You're adding a new ChangeMode while toggling is essentially setting the value to its opposite:
Ex. toggle flying state
This should not be handled as 'ChangeMode.TOGGLE' but rather as 'ChangeMode.SET'.
Only booleans can be toggled, unless someone can give me an example where another object could be toggled. The pattern should become toggle %booleans%, which essentially means that whenever a boolean expression can be set to something, it can also be set to its opposite, which is essentially what toggling means.

Olyno · 2021-07-03T22:06:39Z

The pattern should become toggle %booleans%

Yeah, but here is the problem: how do you manage that with addons?

Mwexim · 2021-07-03T22:23:27Z

The pattern should become toggle %booleans%

Yeah, but here is the problem: how do you manage that with addons?

Addons that register boolean expressions can just accept SET as a valid change mode?

OGContent · 2021-07-03T22:56:17Z

Addons that register boolean expressions can just accept SET as a valid change mode?

Hey, I don't really get what you mean, why shouldn't TOGGLE be a mode?
also do you mean the TOGGLE should be like set {variable} to toggle {variable} or what?

Mwexim · 2021-07-04T08:30:21Z

Addons that register boolean expressions can just accept SET as a valid change mode?

Hey, I don't really get what you mean, why shouldn't TOGGLE be a mode?
also do you mean the TOGGLE should be like set {variable} to toggle {variable} or what?

Not really. I am advocating for the removal of this ChangeMode because toggling equals setting a value to its opposite. This means that all boolean expressions that accept 'set' as a valid ChangeMode, can be toggled, since, again, toggling equals setting a value to its opposite.

AyhamAl-Ali · 2021-07-04T08:56:20Z

I think both ways are useful as a GhangeMode and as an expression.

because toggling equals setting a value to its opposite.

I don't think this is correct, it is technically setting to it's opposite but to do this manually you will need a work-around and it will be longer that just toggle ... or toggled ... which makes it much easier if you use it a lot.

Mwexim · 2021-07-04T09:14:00Z

I think both ways are useful as a GhangeMode and as an expression.

because toggling equals setting a value to its opposite.

I don't think this is correct, it is technically setting to it's opposite but to do this manually you will need a work-around and it will be longer that just toggle ... or toggled ... which makes it much easier if you use it a lot.

You are not understanding me correctly but at this point I think it doesn’t matter anymore.

AyhamAl-Ali · 2021-07-04T09:20:36Z

You are not understanding me correctly but at this point I think it doesn’t matter anymore.

I would like to understand your opinion of course, let me know more. It does matter :)

AyhamAl-Ali · 2021-07-04T09:25:28Z

My point is that you can toggle using current SET mode like this

set flight state of player to false if flight state of player is true else true
# OR
if flight state of player is true:
  set flight state of player to false
else:
  set flight state of player to true

Which works perfectly but adding

toggle flight state of player
send "Flight state has been set to %toggle flight state of player%"
# OR
set flight state toggled {Flight::%uuid of player%}

Makes it easier in some cases if not many, Am I correct?

TPGamesNL · 2021-07-04T09:27:57Z

@Mwexim's point is that toggle flight state of player does not need a new ChangeMode, but can be done with using get methods and the SET ChangeMode

AyhamAl-Ali · 2021-07-04T09:41:14Z

@Mwexim's point is that toggle flight state of player does not need a new ChangeMode, but can be done with using get methods and the SET ChangeMode

But with that it won't be possible to do toggle flight state of player because toggle is GhangeMode here right?

TPGamesNL · 2021-07-04T09:43:13Z

But with that it won't be possible to do toggle flight state of player because toggle is ChangeMode here right?

Yes that would be possible, TOGGLE is not a ChangeMode in that code: https://pastebin.com/mEvXw429

AyhamAl-Ali · 2021-07-04T09:56:38Z

Yes that would be possible, TOGGLE is not a ChangeMode in that code: https://pastebin.com/mEvXw429

If that works well I think it is a better idea, because it will be easier and no need to add TOGGLE ChangeMode to all boolean type expressions manually.

Olyno · 2021-07-04T14:33:09Z

expression.change(e, new Boolean[] {!b}, ChangeMode.SET);

Oh okay, that's what I didn't understand, how we could do that without a mode. It seems to be a good way to do it. I'm gonna probably change for that (and adding a new expression if the community wants to)

Olyno · 2021-07-16T09:33:57Z

This pull request is not ready to be review due to some changes to do. I convert it to draft during this time.

Moderocky · 2023-10-14T09:53:03Z

The change effect already has one pattern that has two expressions next to each other

Just because we made a mistake before doesn't mean we should do it again.

Moderocky

Good

APickledWalrus

mainly formatting/style recommendations.

src/main/java/ch/njol/skript/effects/EffToggle.java

APickledWalrus · 2024-05-29T16:37:41Z

src/main/java/ch/njol/skript/effects/EffToggle.java

+		List<Object> filteredValues = toggledValues.stream().filter(
+			obj -> ChangerUtils.acceptsChange(toggledExpr, ChangeMode.SET, obj.getClass())
+		).collect(Collectors.toList());
+		toggledExpr.change(event, filteredValues.toArray(), ChangeMode.SET);


We might need to note a behavior change here. For changing something like a list, I believe the indexes will now be lost, which was not prior behaviors. We might choose to only rewrite the list if it contains a boolean?

When you're talking about the list, are you talking about the toggledExpr or the filteredValues, or even both?

if we have a list like {_values::*} with:
{_values::pickle::name} = "APickledWalrus"
{_values::pickle::likesCake} = true
if we then do toggle {_values::*}, the indices such as name and likesCake will be lost (replaced by 1 and 2).
This is probably unavoidable. However, if we have a list like
{_values::pickle::name} = "APickledWalrus"
{_values::pickle::githubName} = "APickledWalrus"
if we then do toggle {_values::*}, there are no booleans to toggle, meaning we should not update the list (and cause it to lose its indices)

Looking at the code, don't i already do it?

https://github.com/SkriptLang/Skript/pull/4154/files#diff-4cdf11cdcad1ff81963a3380536868678c5781162395d093ecd42b33c71add01R74

Also, tests are failing, did i misunderstand what you meant?

There may have been some confusion, sorry.

Given a list containing "one" and "two", doing toggle {_list::*} should not modify the list since there is nothing to toggle (meaning the indexes will be preserved).

However, given a list containing "one", "two" and true, doing toggle {_list::*} should modify the list since the third value is a boolean (meaning the indexes will be lost).

I think the fix you made is correct, but the test is wrong.

Working with the testing you added, you should be testing:

set {_list::myFirstIndex::myFirstValue} to "hello" set {_list::mySecondIndex::mySecondValue} to "world" toggle {_list::*} assert {_list::myFirstIndex::myFirstValue} is "hello" with "{_list::myFirstIndex::myFirstValue} should be toggled to 'hello'" assert {_list::mySecondIndex::mySecondValue} is "world" with "{_list::mySecondIndex::mySecondValue} should be toggled to 'world'"

Update: shouldn't we throw an error instead? If the user tries to toggle strings, it shouldn't work. If the variable list includes string, but also boolean, shouldn't we throw an error? Note that i don't think we should throw an error if all values are togglables.

Well, we can't really do runtime errors. I don't think it is just an issue to ignore the values and only change the ones that are actually togglable. You could also opt to not make any changes if there is an invalid type.

The main point still stands, though. If the list contains only blocks, we do not want to actually call the change method as it will reset the indices.

Can you confirm me the commit 8bc1de4 fixes this unexpected behavior?

it looks reasonable
you may want to check out #7120 changeInPlace utility method

I would need a usage example please. That being said, i will wait for this PR to be merged before doing any change regarding this change.

In terms of organization, either the changeInPlace PR is merged before this one, and so i will apply the change here, or this PR is merged before, and so i will apply the change in the other PR.

In any case, we need to merge one of them to move on the other.

Co-authored-by: Patrick Miller <[email protected]>

src/main/java/ch/njol/skript/effects/EffToggle.java

Olyno added 4 commits July 3, 2021 16:45

feat(changemode): introduces toggle mode

ddb40a8

merge: master

7c6dfb5

fix(unreachable): remove unecessary statement

f5d6762

fix(unreachable): remove unecessary statement

9f53cc5

TPGamesNL requested changes Jul 3, 2021

View reviewed changes

Olyno and others added 4 commits July 3, 2021 22:16

fix(condition): missing pipe sign

7c799fe

Co-authored-by: TPGamesNL <[email protected]>

revert(ExprWhitelist): reset mode affected

62af771

fix(ExprGlowing): missing toggle mode in acceptChange

717cb84

Merge branch 'feature/toggleChanger' of github.com:Olyno/Skript into …

c9c87be

…feature/toggleChanger

Merge branch 'master' into feature/toggleChanger

59f8e68

TPGamesNL added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Jul 6, 2021

Olyno marked this pull request as draft July 16, 2021 09:34

Merge branch 'master' into feature/toggleChanger

e6ddeea

This comment was marked as off-topic.

Sign in to view

Merge branch 'dev/feature' into feature/toggleChanger

c20f01f

Moderocky approved these changes Apr 3, 2024

View reviewed changes

sovdeeth added the feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. label Apr 4, 2024

Olyno and others added 4 commits April 8, 2024 02:34

Merge branch 'dev/feature' into feature/toggleChanger

14cc4e0

Merge branch 'dev/feature' into feature/toggleChanger

fa7d13c

Merge branch 'dev/feature' into feature/toggleChanger

fc6840c

Merge branch 'dev/feature' into feature/toggleChanger

6cde78c

APickledWalrus requested changes May 29, 2024

View reviewed changes

Olyno and others added 9 commits June 3, 2024 19:35

chore(formatting): fix formatting

5d5cb4a

Co-authored-by: Patrick Miller <[email protected]>

chore(formatting): fix formatting

9bb3ed5

Co-authored-by: Patrick Miller <[email protected]>

chore(formatting): fix formatting

ae5960f

Co-authored-by: Patrick Miller <[email protected]>

chore(formatting): fix formatting

8ee2c1b

Co-authored-by: Patrick Miller <[email protected]>

chore: collect as array directly

1315872

Co-authored-by: Patrick Miller <[email protected]>

Merge branch 'dev/feature' into feature/toggleChanger

bbabd72

chore: rename 'toggledExpr' to 'togglables'

b5dd7b5

fix(toggle): try to fix unexpected behavior

41a45fe

Merge branch 'dev/feature' into feature/toggleChanger

15a8bf2

APickledWalrus requested changes Jun 20, 2024

View reviewed changes

src/main/java/ch/njol/skript/effects/EffToggle.java Show resolved Hide resolved

src/main/java/ch/njol/skript/effects/EffToggle.java Outdated Show resolved Hide resolved

APickledWalrus removed the feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. label Jul 1, 2024

Merge branch 'dev/feature' into feature/toggleChanger

7259c54

APickledWalrus mentioned this pull request Sep 5, 2024

Flipped Boolean Expression #7039

Closed

1 task

Olyno added 4 commits November 15, 2024 23:26

Merge branch 'dev/feature' into feature/toggleChanger

565fe7d

chore(EffToggle): check if accepts more than single value

ab9b399

fix(EffToggle): avoid index reset when possible

8bc1de4

fix(test): rephrase error

adf2438

Introduces toggle effect & expression #4154

Are you sure you want to change the base?

Introduces toggle effect & expression #4154

Conversation

Olyno commented Jul 3, 2021 • edited Loading

Description

Mwexim commented Jul 3, 2021 • edited Loading

TPGamesNL left a comment

Choose a reason for hiding this comment

Olyno commented Jul 3, 2021

Mwexim commented Jul 3, 2021

Olyno commented Jul 3, 2021

Mwexim commented Jul 3, 2021

OGContent commented Jul 3, 2021 • edited Loading

Mwexim commented Jul 4, 2021

AyhamAl-Ali commented Jul 4, 2021

Mwexim commented Jul 4, 2021 • edited Loading

AyhamAl-Ali commented Jul 4, 2021

AyhamAl-Ali commented Jul 4, 2021

TPGamesNL commented Jul 4, 2021

AyhamAl-Ali commented Jul 4, 2021

TPGamesNL commented Jul 4, 2021

AyhamAl-Ali commented Jul 4, 2021

Olyno commented Jul 4, 2021

Olyno commented Jul 16, 2021

Moderocky commented Oct 14, 2023

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

Moderocky left a comment

Choose a reason for hiding this comment

APickledWalrus left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Olyno commented Jul 3, 2021 •

edited

Loading

Mwexim commented Jul 3, 2021 •

edited

Loading

OGContent commented Jul 3, 2021 •

edited

Loading

Mwexim commented Jul 4, 2021 •

edited

Loading