Skip to content
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.

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

CoordCellRef: Fix adding the optional finder #574

Open
wants to merge 4 commits into
base: maintenance/mps20213
Choose a base branch
from

Conversation

alexanderpann
Copy link
Member

This PR replaces the not working optional finder cell of the CoordCellRef editor with a transformation menu so that the finder can be added through the code completion menu of the cell instead of typing "/" before the cell.
I changed it because I couldn't get the optional cell working. The missing spaces in the editor (punctuation-x: true) prevent the side transformation to be executed. Fixes #559.

@brucetrask
Copy link

@alexanderpann and @sergej-koscejev Thanks very much for the fix. I tried it and it works great. Should we create an issue for the broken optional cell in the grammar cells language? I don't know where that language is or know much about it but I heard it is something useful I should be using. Is it also part of iets3.opensource?

@alexanderpann
Copy link
Member Author

@alexanderpann and @sergej-koscejev Thanks very much for the fix. I tried it and it works great. Should we create an issue for the broken optional cell in the grammar cells language? I don't know where that language is or know much about it but I heard it is something useful I should be using. Is it also part of iets3.opensource?

Actually, the old behavior of the optional cell was wrong, which was fixed a while ago. The way the cell was used is just not compatible with the latest version of grammar cells. There is nothing else that needs to be fixed.

@brucetrask
Copy link

brucetrask commented Jun 28, 2022

@alexanderpann and @sergej-koscejev Ok. Thanks. One last follow up just to make sure I am understanding (I am a bit new to MPS). So, could I use the optional grammar cell facility in this sheets use case or is it that use case which is not compatible with the grammar cells I have with 2021.1? In other words, you mentioned you couldn't get the optional cell working in your comment above. Is that because the use in the sheets is not possible or something else? Just wanted to understand.

@alexanderpann
Copy link
Member Author

The optional cell uses side transformations. Normally they work, but this editor uses punctuation-left:true and punctuation-right:true in some cells which disallows the cursor at those positions. As a result, the side transformations can't be executed because you are not allowed to enter the triggering text (in this case: "/").

@brucetrask
Copy link

Thanks @alexanderpann . Appreciate your help on this.

Copy link
Contributor

@wsafonov wsafonov left a comment

Choose a reason for hiding this comment

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

Looks good in general. Just couple of minor questions:

  • is there really a need for the additional vertical collection wrapper in the editor?
  • in case there is a already cell specified without a finder, would just typing / before that add the finder as expected? I'm not sure if this worked before with an optional cell, but just for the sake of completeness.
  • would it be possible to add some editor tests for this scenario? To ensure we avoid regression in this specific editor behaviour in the future.

@alexanderpann alexanderpann requested a review from wsafonov July 25, 2022 11:05
@alexanderpann alexanderpann changed the base branch from master to maintenance/mps20211 October 12, 2022 12:22
@arimer
Copy link
Member

arimer commented Feb 13, 2023

@alexanderpann I am not sure here. Where the questions/findings of @wsafonov already addressed in this PR or are they missing?

@alexanderpann
Copy link
Member Author

IMO, I've addressed all points.

@alexanderpann alexanderpann force-pushed the bugfix/SpreadsheetAddSheet#559 branch from edd6322 to ad0194d Compare March 10, 2024 15:30
@alexanderpann alexanderpann changed the base branch from maintenance/mps20211 to maintenance/mps20213 March 10, 2024 15:30
@alexanderpann
Copy link
Member Author

@arimer I've rebased the PR for 2021.3. It could be merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spreadsheets: adding sheet with '/' does not work
4 participants