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

Tables: Various fixes to table end cells in combination with insert and deletion handler #974

Merged
merged 22 commits into from
Oct 14, 2024

Conversation

alexanderpann
Copy link
Collaborator

@arimer to test the changes, please have a brief look at the MPS-Extensions Tables Sandbox to check for broken stuff. I didn't add any delete or insert handlers there.

For proper testing, you can open https://github.com/IETS3/iets3.opensource/tree/bugfix/tables as a second project and check instances of the following concepts: DataTable, Sheet, LookupTable, DecTab.

When the default implementation for deletion is used (not derived from a header deletion handler), there is a differentiation between BACKSPACE (delete to the left of cursor) and DELETE (delete to the right of the cursor). For the derived delete actions we could create a followup ticket. For INSERT (enter) and INSERT_BEFORE (Shift + Enter) the same applies. For all actions this means that depending on the ACTION it modifies the row before the current row, the current row or the next row. The left and right cell therefore modify different rows. Example:
DELETE on a left end cell deletes the current row since it is to the right of the cell, DELETE on a right end cell deletes the next row.

I didn't open a PR in IETS3 yet because without this PR the other PR doesn't build. I also added a few helper methods such as Grid#setEndCellAction that are unused in both repos as the moment but could be useful for other people.

arimer and others added 19 commits September 24, 2024 15:22
updated Runtime to support EndRowCreation also for Tables with row headers
This way, you can still overwrite them in a post-processor.
Copy link
Collaborator

@arimer arimer left a comment

Choose a reason for hiding this comment

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

Awesome refactoring 👍🏼
Just some minor findings we should address before merging:

  1. lets surround the whole logic inside the for loop with that header!=null check
  2. Inside the ActionSettingAdapter, this action is not matching properly the case condition. I guess it should be a setDeleteAction()
  3. We are under rightEnabled. How about we rename this variableto something "right"
  4. This HeaderReference key might be not stable enought. Should we change the getFallBackText()to also take the value into account?

@arimer
Copy link
Collaborator

arimer commented Oct 10, 2024

The usecases in iest3 tables work as epected 💯

However I gave up on understanding why a lookup table looks the way it looks like:
grafik

I don't know why there is the 2 literal at all and why the end-cell is not show there, but this should not hinder us in merging.

@alexanderpann
Copy link
Collaborator Author

alexanderpann commented Oct 14, 2024

How about we rename this variableto something "right"

We still query the left create handler, so the name is fine.

hould we change the getFallBackText()to also take the value into account?

I changed SNodeHeaderReference#getFallBackText to match StringHeaderReference#getFallBackText.

I don't know why there is the 2 literal at all and why the end-cell is not show there, but this should not hinder us in merging.

Agree but you can't insert a row here anyway, so it is correct.

@arimer arimer self-requested a review October 14, 2024 07:31
Copy link
Collaborator

@arimer arimer left a comment

Choose a reason for hiding this comment

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

Thanks for the cleanup 👍🏼

Changes look fine now.

@alexanderpann alexanderpann merged commit 736f389 into maintenance/mps20222 Oct 14, 2024
1 check passed
@alexanderpann alexanderpann deleted the bugfix/tablesGen branch October 14, 2024 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants