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.

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

Add option to merge cell text #539

Merged
merged 1 commit into from
Oct 13, 2023
Merged

Add option to merge cell text #539

merged 1 commit into from
Oct 13, 2023

Conversation

tobiolo
Copy link
Collaborator

@tobiolo tobiolo commented Oct 10, 2023 •
edited
Loading

This adds a option to merge cells.

@tobiolo tobiolo force-pushed the merge-text branch 7 times, most recently from 83b1ad8 to d610857 Compare October 11, 2023 12:09
@aardappel
Copy link
Owner

Like I said in the other PR, this is a bit or a weird operation: "That is simpler, but also not so useful.. there is already a function to copy merged text, so the user can simply just use that to do the operation manually, copy the text, delete, and insert."

There's also the issue of UI clutter.. the Edit menu keeps growing, and now we have 2 operations that do almost the same thing, and are both quite specialized/niche.

@tobiolo
Copy link
Collaborator Author

tobiolo commented Oct 11, 2023 •
edited
Loading

Like I said in the other PR, this is a bit or a weird operation: "That is simpler, but also not so useful.. there is already a function to copy merged text, so the user can simply just use that to do the operation manually, copy the text, delete, and insert."

There's also the issue of UI clutter.. the Edit menu keeps growing, and now we have 2 operations that do almost the same thing, and are both quite specialized/niche.

Well I do slightly disagree:
As you wrote, it would require three manual steps instead of just one as this proposal would also delete cells. I don't think it is so niche as you might think. Just think of someone who pastes a multiline text that gets split up in TreeSheets but the user wants to keep it in one cell. I think this use case is rather common. Can you see my point?

@aardappel
Copy link
Owner

Hmm, maybe we can try it, but lets use a clearer name for it, maybe "Collapse Cells", to make it clearer cells will be modified.
Also I don't see how your call to delete cells guarantees that the first cells remains? I guess because it contains text?

@tobiolo tobiolo force-pushed the merge-text branch 2 times, most recently from cb7dfa0 to 54a0d25 Compare October 12, 2023 19:33
@tobiolo
Copy link
Collaborator Author

tobiolo commented Oct 12, 2023

Hmm, maybe we can try it, but lets use a clearer name for it, maybe "Collapse Cells", to make it clearer cells will be modified. Also I don't see how your call to delete cells guarantees that the first cells remains? I guess because it contains text?

Hello Wouter
Oh I missed that spot! I updated the PR to make sure that the first cell has always some content.
Thanks for your review :-) I also changed the name for the function as you proposed.
Kind regards
Tobias

@tobiolo tobiolo force-pushed the merge-text branch 3 times, most recently from f1e176d to bd904a0 Compare October 12, 2023 19:54
aardappel
aardappel reviewed Oct 12, 2023
View reviewed changes
src/document.h Outdated
case A_COLLAPSE: {
if (selected.xs * selected.ys == 1) return _(L"More than one cell must be selected.");
auto fc = selected.GetFirst();
if (!fc->HasContent()) return _(L"The first cell in selection must have content.");
Copy link
Owner

Choose a reason for hiding this comment

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

Why is this? even if this cell has no content, aren't you adding the text of all other cells to it below?
What I meant was I wasn't sure if MultiCellDeleteSub keeps this cell, but I guess it does?

Copy link
Collaborator Author

@tobiolo tobiolo Oct 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The problem arises when you have cells in the selection that all have no content (stupid case but can happen) and the selection is surrounded by cells without content. Then MultiCellDeleteSub will also delete the row/column of the first cell, leading to a crash because of the dangling fc pointer...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right we can check after collecting the text. If there is still no content, then leave the routine...

@aardappel
Copy link
Owner

LGTM!

@aardappel aardappel merged commit 50e1fad into aardappel:master Oct 13, 2023
4 checks passed
@tobiolo
Copy link
Collaborator Author

tobiolo commented Oct 13, 2023

LGTM!

Merci :-)

@tobiolo tobiolo deleted the merge-text branch November 2, 2023 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aardappel aardappel aardappel left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tobiolo @aardappel