Skip to content

+Add en passant option in Board Editor #14165

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 12 commits into from
Dec 7, 2023
Merged

+Add en passant option in Board Editor #14165

merged 12 commits into from
Dec 7, 2023

Conversation

@TheMadSword
Copy link
Contributor

@TheMadSword TheMadSword commented Dec 6, 2023 •
edited
Loading

Fixes #14139 .

Some notes :

  • Bug (fixed) mentioned on ctrl.ts#103 is when [without those 2 lines] you're loading for instance /editor/rnbqkbnr/1p4p1/6P1/4P3/2PPpPp1/1Rpp2P1/8/1NBQKBNR_b_kq_f3_0_1?color=white ; 1- selecting no en passant, 2- re-selecting f3, 3- then moving the d3 pawn to e3 and 4- moving that pawn back [then url doesn't match the "select" visual value].

  • I created also an issue in Have a parseCastlingFen equivalent for enPassant niklasf/chessops#154 regarding having a better en-passant check for chessops (there is a lot of stuff for castling checks, but not much for en passant). I think this could remove the need "fenFixedEp" requirement once implemented (depending on how it's done).

  • Note that I do not allow "half-way" values like the "ahAH" for castling. Either there is one valid en passant value in the fen, or none [or if the fen is illegal, e.g. king in check, then it makes the url differ from the select visual value; which I don't think is problematic].

Otherwise as-is I couldn't find any bug.

What it looks like :
image

@TheMadSword TheMadSword closed this Dec 6, 2023
@TheMadSword TheMadSword reopened this Dec 6, 2023
TheMadSword and others added 2 commits December 6, 2023 02:29
@TheMadSword
formatting the linting
adafde6
@ornicar
Merge branch 'master' into board_editor_add_enpassant
c38fbfc 
* master:
  Add Asturian Flag, ref to lichess-org#14084 (lichess-org#14152)
  refactor flair picker and make it admin aware
  flair/list.sh
  add lichess flair back, for admins only
  move misplaced flair
  refactor flair picker style
  remove debug
  New Crowdin updates (lichess-org#14148)
  Update kamon-core, kamon-influxdb, ... to 2.7.0
@ornicar ornicar self-assigned this Dec 6, 2023
ornicar
ornicar reviewed Dec 6, 2023
View reviewed changes
ui/editor/src/ctrl.ts Outdated
this.redraw();

//Fix En passant select bug where an option being removed breaks the select
const epSelect = $('.enpassant select')[0] as HTMLSelectElement;
Copy link
Collaborator

@ornicar ornicar Dec 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reading and updating the DOM from a ctrl is too much of a hack

Copy link
Collaborator

@ornicar ornicar Dec 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to find another way, but I can't even manage to reproduce the bug it fixes. Can you help me with that?

Copy link
Contributor Author

@TheMadSword TheMadSword Dec 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it isn't pretty. See comment below.

ornicar
ornicar reviewed Dec 6, 2023
View reviewed changes
Copy link
Collaborator

@ornicar ornicar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a lot of code, but a needed feature

ui/editor/src/ctrl.ts Outdated
this.redraw();

//Fix En passant select bug where an option being removed breaks the select
const epSelect = $('.enpassant select')[0] as HTMLSelectElement;
Copy link
Collaborator

@ornicar ornicar Dec 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to find another way, but I can't even manage to reproduce the bug it fixes. Can you help me with that?

@TheMadSword
Copy link
Contributor Author

TheMadSword commented Dec 6, 2023 •
edited
Loading

Hi @ornicar !

I could reproduce on Firefox/Chromium/Chrome the bug [but not in Safari], the 'selected' (attribute) element isn't shown visually [and selecting it via view.ts @ "selected" attribute (when building the 'h') doesn't visually work]. I wrote the steps in my first comment. I just re-reproduced successfully the bug :
1- Comment lines 104+105
2- load /editor/rnbqkbnr/1p4p1/6P1/4P3/2PPpPp1/1Rpp2P1/8/1NBQKBNR_b_kq_f3_0_1?color=white
3- Click and select no en passant (first empty option)
4- Click and select the f3 option that we just removed at the step 3
5- drag & drop to move the d3 black pawn over to e3
6- drag & drop to move that pawn now at e3, back to d3
7- notice the URL keeps the 'f3' value, while the visually selected value is empty, and the selected (attribute set properly) is good (f3). So we have an option 'selected' value not visible. [step 6 removed the 'd3' option of en-passant, which created the bug I'd presume].

Video + img of dom-elements

Screen.Recording.2023-12-06.at.1.03.57.PM.mov
image

@TheMadSword
Copy link
Contributor Author

TheMadSword commented Dec 6, 2023 •
edited
Loading

@ornicar

I just found a way to fix the bug without using those two lines. It involves the 'hidden' attribute and having all [16] possible values at all time (but hidden if unused ). Since no value is removed, it doesn't break the select. I'll commit once master and/or the branch is compilable [getting [error] -- [E046] Cyclic Error: /Users/redmba/dev/myLichess/lila/modules/common/src/main/Chronometer.scala:41:31 at the moment] to not lose your modifs.

@lenguyenthanh
Copy link
Member

lenguyenthanh commented Dec 6, 2023

I'll commit once master and/or the branch is compilable [getting [error] -- [E046] Cyclic Error: /Users/redmba/dev/myLichess/lila/modules/common/src/main/Chronometer.scala:41:31 at the moment] to not lose your modifs.

I had the same issue, but clean;compile fixed it.

@TheMadSword
~Fixed differently the en passant select bug
7275cb3 
~increased to 9ch select size (for chrome)
@TheMadSword
Copy link
Contributor Author

TheMadSword commented Dec 7, 2023 •
edited
Loading

I'll commit once master and/or the branch is compilable [getting [error] -- [E046] Cyclic Error: /Users/redmba/dev/myLichess/lila/modules/common/src/main/Chronometer.scala:41:31 at the moment] to not lose your modifs.

I had the same issue, but clean;compile fixed it.

Thanks lenguyenthanh :) ! (my bad !)

@ornicar

I implemented the fix I mentioned earlier : using all the values being always present. A few things to note :

  • Under Safari, the option cannot be hidden/display:none… I decided to disable them, instead of make a "if browser" in typescript, which would also not be pretty (and much worse).
  • I could decide to change the en passant string[] for a Set (@ enPassantOptions), but looking at statistics, Array seems faster for lookup when array is < 20 elements, so it makes sense to keep it.
  • I increased the select width from 8ch to 9ch for "d3" and "b3" (among others; which was too small before on chrome, but not firefox).
  • "'' && true", and "'' && false" return both '', so "Boolean" function is required (for '').

I hope everything is now good. Tell me if I should do other modifs. Thanks !

@ornicar
Merge branch 'master' into board_editor_add_enpassant
5aec8e4 
* master:
  show player flag on its powertip
  tweak team cache expiration
  clear LightTeam cache on flair update
  detect unsaved form changes, prompt the user - closes lichess-org#14095
  make sure dialog is no wider than the screen, add text ellipsis
  fix /paste double warning, tweak style
  show when kid mode is enabled
  merge newPlayer template
  scala tweaks
  remove bad translation lego - not necessary anyway
  split new translations to onboarding.xml
  translate flair
  delete unused removeComputerVariations function
  pointlessly switch boolean tests to reduce function calls
  hide rather than clear server eval when analysis disabled
  bigger activation window for well-intentioned but occasional streamers
  Add translate new player landing page
@ornicar
TS code golf
abb1145
@ornicar
Copy link
Collaborator

ornicar commented Dec 7, 2023

Excellent stuff, thanks!

@ornicar ornicar merged commit 5f8752e into lichess-org:master Dec 7, 2023
@TheMadSword TheMadSword mentioned this pull request Dec 8, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ornicar ornicar ornicar left review comments

Assignees

@ornicar ornicar

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Can't set en passant in the board editor

3 participants

@TheMadSword @lenguyenthanh @ornicar