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

Do not insert ebut data section when looking for it #316

Merged
merged 1 commit into from
Apr 22, 2023

Conversation

matsl
Copy link
Collaborator

@matsl matsl commented Apr 8, 2023

What

Do not insert ebut data section when looking for it.

Why

A data section is inserted when executing buttons in non file buffers such as scratch, news summary or temp buffers. Test case for temp buffer with non existing ebut added. (Not an ideal test case since verifying that something does not happen is not the best test. The test can be applied locally before trying the fix to demonstrate that it happens.)

The fix is more of a hack. It locally check if the create flag is set, using the fact it should not be set when only looking for if button data exists!?, and if it is not set avoids the insertion.

But maybe we should refactor this part of the code? It feels odd to me that ebut:at-p would even come close to code that does insertion. Also the explicit checks in there on the email and news interface looks like a code smell to me.

@matsl matsl force-pushed the remove_bd_data_section_creation_when_executing_button branch 2 times, most recently from 3562f5d to 079f159 Compare April 8, 2023 23:22
@matsl matsl requested a review from rswgnu April 8, 2023 23:24
@matsl matsl force-pushed the remove_bd_data_section_creation_when_executing_button branch from 079f159 to 03e8d21 Compare April 9, 2023 14:38
@rswgnu
Copy link
Owner

rswgnu commented Apr 15, 2023

I believe I fixed this with changes that I pushed last week. So if this works on master, just remove this branch without deploying.

Copy link
Owner

@rswgnu rswgnu left a comment

Choose a reason for hiding this comment

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

Drop this pull request; fixed elsewhere.

@rswgnu rswgnu closed this Apr 15, 2023
@matsl matsl reopened this Apr 15, 2023
@matsl matsl force-pushed the remove_bd_data_section_creation_when_executing_button branch from 03e8d21 to 6c60b13 Compare April 15, 2023 19:32
@matsl
Copy link
Collaborator Author

matsl commented Apr 15, 2023

@rswgnu The test case hypb--ebut-at-p-should-not-insert-hbdata-section-in-non-file-buffers still fails on the master branch. So I brought the PR back and rebased. I have also verified manually that the separator is still inserted on the master branch.

@rswgnu
Copy link
Owner

rswgnu commented Apr 15, 2023

The ebut separator is supposed to be added to buffers without files such as email and lisp interaction buffers, so it seems to me the test case is not correct.

Adding the 'create' flag usage looks correct.

@matsl
Copy link
Collaborator Author

matsl commented Apr 15, 2023

Why should anything be added when checking if there is a button or trying to use what looks like a button? Should not that be a read only operation? That the ebut separator is missing should not that be taken as an indication that there is no valid button rather than inserting it just to realize that there actually is no button data?

@matsl matsl force-pushed the remove_bd_data_section_creation_when_executing_button branch from 6c60b13 to 069a459 Compare April 21, 2023 23:07
@matsl
Copy link
Collaborator Author

matsl commented Apr 21, 2023

@rswgnu I have some vague memory that you wanted something more to be added to this PR but coming back to this now for the weekend I can't remember what it was.

@rswgnu
Copy link
Owner

rswgnu commented Apr 22, 2023 via email

@matsl matsl merged commit 2dae930 into master Apr 22, 2023
4 checks passed
@matsl matsl deleted the remove_bd_data_section_creation_when_executing_button branch April 22, 2023 17:32
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.

None yet

2 participants