+AutoZen (Closes #12537) #13154

TheMadSword · 2023-07-03T07:21:34Z

-Rem one unused arg in radios

* master: (55 commits) globally import cats.{Eq, Show, data.NonEmptyList} scala tweaks fix Long to Int globally import cats.syntax.all.* tweak tournament top cache scala tweaks use union type after lichess-org#13134 neater cats syntax after lichess-org#13134 New Crowdin updates (lichess-org#13167) use ReadPref.priTemp Fix wrong rewrite by scalafmt fix ublog page title when editing Update git blame ignore revs Fix scalafmt crashes Run scalafmtAll again Fix compiling error Run scalafmtAll reduce some youtube streamer log statement priorities Bump scalafmt 3.7.7 and add rewrite config remove last references to ReadPreference ...

ornicar · 2023-07-06T11:12:00Z

modules/pref/src/main/Pref.scala

+          value match 
+            case "1" => 1
+            case "2" => 2
+            case "3" => 3


3? in which case do we want to set it to 3?

Hi,

I was trying to do as you said here : #12537 (comment)

To use 4 values of zen, while displaying 3 in the preference menu/ui. The user, if let's say he's in a puzzle and press 'z' while on value '2', would be having it's zen set to 3.

ornicar · 2023-07-06T11:21:59Z

app/views/account/bits.scala

  def setting(name: Frag, body: Frag) = st.section(h2(name), body)

-  def radios[A](field: play.api.data.Field, options: Iterable[(A, String)], prefix: String = "ir") =
+  def radios[A](field: play.api.data.Field, options: Iterable[(A, String)], valueAliases: Option[Map[Option[String], Option[String]]] = None) =


(following the other comment above)

The valueAliases is specifically to to make is so what of those 4 values is equaled to the other (3->2) for the user when he looks at the menu of pref/ui. This allows to keep the old behavior of zen toggling.

then it would be better to avoid using this radios function, rather than adding this strange complexity no other caller needs.

I see; would making a function radios_with_aliases do the job ?

ornicar · 2023-07-06T11:22:14Z

app/views/account/bits.scala

-        val id      = s"$prefix${field.id}_$key"
-        val checked = field.value has key.toString
+        val id      = s"ir${field.id}_$key"
+        val checked = (field.value has key.toString) || (~((~valueAliases).get(field.value)) has key.toString)


not sure what's going on here but there has to be a better way

(following the other comment above)

By splitting the pref into 2 bools, I am able to do like you said in your comment in the issue, to have 4 separate values, while being able to toggle one of those values (so zen in game being automatic remains independant from using zen in another place)

radios_with_aliases is not a scala function name, look around for conventions.

Option[Map[Option[String], Option[String]]] looks very wrong and causes the implementation to be unnecessary complicated

ornicar · 2023-07-06T11:23:44Z

app/controllers/Pref.scala


+  private def saveZen()(value: String, ctx: Context): Fu[Cookie] =
+    //Discard 0-1 bit, then add 0-1 bit with wanted value
+    var newVal = ctx.pref.zenInt & ~1 | value.toInt


this is quite cryptic, even with the comment. Why?

Zen.GAME_AUTO (=2) was initially Zen.Automatic_Bit [with Zen.Game_On ('3') and Zen.Game_Off, with same '2' value] when I started developping it ; this helped understand that it is a bit functionality (I had 3 var, but 2 were unused so no point in keeping them in the end).

This makes it so the user can keep toggling it's zen, and that whatever he does, when he toggles it, it will only affect the "yes-no" part of it, and never the "Automatic-in-game" part of it (while can only be activated/deactivated via the pref menu).

This reduces the knowledge one needs regarding toggling zen, and allows to not have on every zenable page conditionnal setting of value (e.g. if zen is automatically-in-game or not).

I see but the problem with this is that it leaks the zen storage binary subtleties to the Pref controller.

Do you mean file-wise or execution-wise ? Would moving saveZen in a helper file solve this ? Or should I put more conditional logic in the .ts ?

ornicar · 2023-07-06T11:24:50Z

Not a fan of what that introduces in the code. I can already hear future me swear while refactoring it.

ornicar · 2023-07-06T11:26:37Z

Here's the explanation on the issue page:

The way I handled it :
I'm giving the zen class to the game body upon game start (without saving the pref), and when toggling Zen, in game, it works normally but the Pref save is prevented (to "separate" the zen toggling of the game [as well as other section of the website] from the zen of the prefs). At the end, if we're in automatic mode, it reverts to non-zen (again without modifying the Prefs).
I could have also saved automatically zen (via Prefs) on game start & end, but I think it makes more sense to spam less the servers to do this other way.

Somehow that doesn't help me understand what's going on here. Only thing I can say is that indeed saving zen on each game start and end would be a terrible idea.

TheMadSword · 2023-07-06T19:51:36Z

Not a fan of what that introduces in the code. I can already hear future me swear while refactoring it.

Would you prefer if I removed the saveZen scala logic (app/controllers/Pref.scala would be fully like before) and push more conditionnal logic into the various zenable .ts (change the various "const zen = $('body').toggleClass('zen').hasClass('zen'); [...] setZen(zen);") ?

EDIT : Also possibly one part of the misunderstanding is that I'm trying to keep the old Zen behavior, because I still believe that someone may want for instance to use zen doing puzzle, while still keeping it automatic in games. Do we want to prevent that and force in-game only and prevent zen mode outside of games with Zen-mode auto ?

ornicar · 2023-07-07T07:04:13Z

If the user selects "In-game only" then it's fair to only set zen in games, and not in puzzles. The use case here is probably to avoid being distracted by opponent ratings, something specific to games.

TheMadSword · 2023-07-07T07:15:29Z

If the user selects "In-game only" then it's fair to only set zen in games, and not in puzzles. The use case here is probably to avoid being distracted by opponent ratings, something specific to games.

By this do you mean to prevent using 'zen' in puzzles ?

As I understand, and as you said here #12443 (comment) it may breaks user workflow when not allowing the togglability/toggling zen anymore. Plus what if you're in game and cannot use zen anymore ? (Since from what you're saying is to enforce the "In-game only" from what I'm understanding; I still believe the user should be able to toggle everywhere)

Shouldn't 'z' / zen toggle be kept everywhere ?

ornicar · 2023-07-07T08:52:39Z

Yes we want to preserve the feature in every place of the site that supports it. I was saying that if the user selects "in-game only" then it's ok to only use it in games.

TheMadSword · 2023-07-08T02:24:06Z

Hi @ornicar ! Thanks for the comments :) !

1- I am a bit confused…

"then it's fair to only set zen in games" do you mean to preserve the feature/toggling everywhere only when zen = 0/1 ? So you'd mean to disable the zen toggle outside games, when zen = 2 or 3 ?

Perhaps I should rename the button "in-game only" to "Automatic in-game" then, and also keep zen every where in every cases. That way there is less loss of functionality for the user everywhere.

Because the issue itself isn't to make it only in-game, but to make it toggle automatic in-game…

2-   I did rename "radios" function to "radios_with_aliases".

3- Could you tell me about #13154 (comment) ?

Thank you !

ornicar · 2023-07-19T07:03:54Z

The whole thing looks sketchy and I feel like it would be better for me to start over and try to find a simple solution, than to try to slowly fix this. Unfortunately I have more important stuff in the oven atm. I'm not even sure an elegant solution that would make the feature simple enough to be acceptable even exists.
Maybe focus on bug fixes, code simplification and general maintenance rather than adding features and complexity.

TheMadSword · 2023-07-22T22:35:34Z

I have in mind of removing the 4th state mentioned (zen==3), and that would simplify everything.

It would remove the pref-controller complexity, the alias complexity and the bit complexity.

Then to compensate I'd just put the "zen-automatic" (if pref ==2) whenever something is zenable, and simply not Xhr call when "zen-automatic". So toggling would still work everywhere I believe, just not be saved in prefs, when in automatic.

I think it would be much more peaceful code-wise.

I'll probably do that on ~next week.

TheMadSword · 2023-07-27T12:49:29Z

Closed in favor of #13304

+AutoZen (closes lichess-org#12537)

72e43c3

-Rem one unused arg in radios

TheMadSword closed this Jul 5, 2023

TheMadSword reopened this Jul 5, 2023

TheMadSword changed the title ~~+AutoZen (closes #12537)~~ +AutoZen (Closes #12537) Jul 5, 2023

ornicar added 2 commits July 6, 2023 12:56

fix master merge

14dbde6

ornicar reviewed Jul 6, 2023

View reviewed changes

ornicar added 2 commits July 6, 2023 13:14

auto-reformat (use scalafmt)

dbbd52a

validate zen input using Zen.choices

550b92d

ornicar reviewed Jul 6, 2023

View reviewed changes

TheMadSword force-pushed the zenautoingame branch from 586e86d to 3a44361 Compare July 8, 2023 01:41

TheMadSword marked this pull request as draft July 8, 2023 02:02

TheMadSword force-pushed the zenautoingame branch from 3a44361 to 27e94d6 Compare July 8, 2023 02:05

~Duplicating "radios" function to remove aliases in the original

c3fc269

TheMadSword force-pushed the zenautoingame branch from 27e94d6 to c3fc269 Compare July 8, 2023 02:06

TheMadSword mentioned this pull request Jul 27, 2023

+AutoZen (Closes #12537) #13304

Merged

TheMadSword closed this Jul 27, 2023

Uh oh!

+AutoZen (Closes #12537) #13154

+AutoZen (Closes #12537) #13154

Uh oh!

Conversation

TheMadSword commented Jul 3, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheMadSword Jul 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ornicar commented Jul 6, 2023

Uh oh!

ornicar commented Jul 6, 2023

Uh oh!

TheMadSword commented Jul 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ornicar commented Jul 7, 2023

Uh oh!

TheMadSword commented Jul 7, 2023

Uh oh!

ornicar commented Jul 7, 2023

Uh oh!

TheMadSword commented Jul 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ornicar commented Jul 19, 2023

Uh oh!

TheMadSword commented Jul 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheMadSword commented Jul 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

TheMadSword Jul 7, 2023 •

edited

Loading

TheMadSword commented Jul 6, 2023 •

edited

Loading

TheMadSword commented Jul 8, 2023 •

edited

Loading

TheMadSword commented Jul 22, 2023 •

edited

Loading