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

[editor] Allow formatting only the selection #10204

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ricardojlrufino
Copy link
Contributor

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Copy link
Collaborator

@matthijskooijman matthijskooijman left a comment

Choose a reason for hiding this comment

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

Looks like a useful feature. I went over the code, it looks good to me (just left some nitpicks inline).

app/src/cc/arduino/packages/formatter/AStyle.java Outdated Show resolved Hide resolved
app/src/cc/arduino/packages/formatter/AStyle.java Outdated Show resolved Hide resolved
@ricardojlrufino
Copy link
Contributor Author

ricardojlrufino commented May 14, 2020

About Eclipse formatter

Some things change unexpectedly, I use Eclipse. And apparently he took the "Arduino" formatter embedded in the settings, so he should obey the formatting ... I will be monitoring ..

One thing that needs to be changed is the maximum number of characters to break the line, 80 is very little, I think it should be 120. I end up not using it because of this limit, which more hinders than helps.

image

@cmaglie
Copy link
Member

cmaglie commented May 15, 2020

Hi @ricardojlrufino !

If I try to format this:

image

I get this:

image

doesn't look correct to me, I expect the indentation to be right. If I select the whole function (up to external { }) then the indentation is correct. I think the context must be taken into account but don't know if this is easy to do with libAStyle

@ricardojlrufino
Copy link
Contributor Author

Thanks, @cmaglie, for the feedback
I did not analyze the library in depth, but looking over it I did not see this option.

I had seen this problem, and I find it easier to correct using TAB to adjust the indentation.
Trying to identify the context, and adjust it automatically, seems like it’s not that easy.

@ricardojlrufino
Copy link
Contributor Author

Hi, i found a hack: Add comments to disable the formatting..

Before

image

After:

image

In this case, you would have to insert these comments dynamically into the text ... and have them formatted, then remove ...
It seems a little ugly, but it was the way I found it

@cmaglie
Copy link
Member

cmaglie commented May 19, 2020

The purpose of this PR is to make things simpler: select a piece of source code and indent just that (this is to avoid the need to indent everything). IMHO it's a useful feature and a good idea, but adding comments to disable autoindent will make the whole experience very awkward, and completely defeat the usefulness of this PR.

If there is no other way to obtain the right user experince I think we should just drop this PR.

@ricardojlrufino
Copy link
Contributor Author

but adding comments to disable autoindent will make the whole experience very awkward

@cmaglie , this would be done programmatically and not by the user

@cmaglie
Copy link
Member

cmaglie commented May 20, 2020

ahhhww, sorry I misunderstood, so your proposed algorithm is:

  1. add indent-off tag at the beginning
  2. wrap user selection between indent-on and indent-off tags
  3. perform auto-indent
  4. removed added indent-off and indent-on tags
  5. redraw

Looking it that way it may work, I'm a bit concerned about the "details", like what happen if I partially select a line? maybe we should indent the whole line even if partially selected.
What if I select a real comment in the source code (how the // INDENT-ON will be inserted in the middle of a /* ... */ comment for example)?

@ricardojlrufino
Copy link
Contributor Author

I managed to implement.
The following tests were done, and they all worked as expected.

image

image

image

image

image

@matthijskooijman
Copy link
Collaborator

What if I select a real comment in the source code (how the // INDENT-ON will be inserted in the middle of a /* ... */ comment for example)?

Did you also test this?

Would be nice to have some actual unittests for this behaviour too. I think there's already one for formatting the entire source, could be added there (see the README for how to run unittests).


// Remove format tags
formattedText = formattedText.replaceAll(Pattern.quote(FORMAT_OFF), "");
formattedText = formattedText.replaceAll(Pattern.quote(FORMAT_ON), "");
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems too general: If the code already contained these format off tags, they would now be removed. I would suggest to remember the offset where the tag was inserted (offset from the end for the third tag), and remove it only from there. As a sanity check, you could check that astyle has indeed left all text up to and including the second tag (and from the third tag to the end) indeed unmodified, otherwise your offsets would not be valid anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem, i put keyword 'DYN' to allow this and not conflict with pre-existing format tags

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I see. I would still think this is somewhat fragile (a user might also put "DYN" in there), but I guess it works well enough in practice. On advantage of doing some offset calculations is that you can at the same time verify that nothing outside of the selection was changed.

formattedText = formattedText.replaceAll(Pattern.quote(FORMAT_ON), "");

textArea.setText(formattedText);
textArea.select(selStart, selStart);
Copy link
Collaborator

Choose a reason for hiding this comment

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

AFAICS, this clears the selection and puts the cursor at the start? Might be good to keep the selection (requires recalculating the end of the selection, but that is probably easier if you solve my previous comment too).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will place the cursor at the beginning of the SELECTION. I would have to recalculate the positioning of the new format, to keep things simple and straightforward I decided not to do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No negative point for usability


// Calculate offsets from begin and end of each line.
int fristLineOffset = textArea.getLineStartOffset(lineStart);
int lastLineOffset = textArea.getLineEndOffset(lineEnd);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this line offset stuff also work correctly when the selection boundaries are just before or just after the newline? Probably something to add a test for all four cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this the example where I need to format 1 line only, and the cursor is at the beginning of the next line? It will force formatting on the 2 lines

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think so, yes. I think I would expect that if I select no characters on the second line, just the newline separating both, I would only format the first line, not the second?

For the start of the selection, it's probably mirrored: If I select just the preceding newline and no characters on the previous line, I would expect to not format the previous line.

@ricardojlrufino
Copy link
Contributor Author

What if I select a real comment in the source code (how the // INDENT-ON will be inserted in the middle of a /* ... */ comment for example)?

NO WOKS ... =[

image

image

@matthijskooijman
Copy link
Collaborator

Hm. One other approach, in theory, is to format the entire file, and then replace everything outside of the selection with the original unformatted code. But since astyle might change the number of lines, it will be impossible to decide which old lines correspond to which new lines...

Any idea what astyle does with unfinished code? E.g. if you pass code that starts at the start of the file, but then ends halfway a function, comment or expression? If astyle handles that properly (as far as possible), then you could:

  1. Format the code before the selection. Save the both the original and the result, but do not apply it.
  2. Format the code from the start of the file to the end of the selection.
  3. Then, in the result of step 2, find the result of step 1 and replace it with the original of step 1.

Effectively, you'll be formatting everything up to the end of the selection and then undoing everything up to the start of the selection.

I guess this can still break, especially if the selection start ends up halfway an expression (then the result of 1 might not be a prefix of the result of 2 because step 2 has more information and might layout code differently), but if you keep the "expand selection to entire lines" step you have now, I suspect that this might actually produce reasonable results (and if it breaks, it can be detected and an error shown).

@ricardojlrufino
Copy link
Contributor Author

I tried your alternative and others, it is complicated to predict all cases.
I ended up finding a simpler solution, is to detect if it is in a comment, and take off until the beginning of it.

Fixed: prevent erros if cursor is in multi-line docs comments, like:
image

I also added new test cases for selection formatting

Stopping here, a lot of work for little gain ..

@matthijskooijman
Copy link
Collaborator

I ended up finding a simpler solution, is to detect if it is in a comment, and take off until the beginning of it.

Hm, I would want to avoid that, since trying to parse source code is really tricky to get right (as can be seen by all the fallout of the autogenerated function prototypes...). Though I guess that just parsing comments can be done without understanding anything else of the code, and just for the formatting, it's ok if it is not entirely perfect...

Good to see tests, they look good at first glance.

@ricardojlrufino
Copy link
Contributor Author

Hm, I would want to avoid that, since trying to parse source code is really tricky

In this case, not... see last commit..
RsyntaxTextArea provide a Token model.. making it much easier to know that the cursor is on top of a comment

@matthijskooijman
Copy link
Collaborator

RsyntaxTextArea provide a Token model.. making it much easier to know that the cursor is on top of a comment

Ah, that makes it a lot better indeed :-)

@CLAassistant
Copy link

CLAassistant commented Apr 9, 2021

CLA assistant check
All committers have signed the CLA.

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.

4 participants