[editor] Allow formatting only the selection #10204

ricardojlrufino · 2020-05-14T04:34:26Z

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?

matthijskooijman

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

ricardojlrufino · 2020-05-14T14:03:27Z

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.

cmaglie · 2020-05-15T08:10:48Z

Hi @ricardojlrufino !

If I try to format this:

I get this:

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 · 2020-05-15T17:51:02Z

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 · 2020-05-15T18:54:54Z

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

Before

After:

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 · 2020-05-19T11:08:08Z

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 · 2020-05-19T21:19:24Z

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 · 2020-05-20T09:17:45Z

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

add indent-off tag at the beginning
wrap user selection between indent-on and indent-off tags
perform auto-indent
removed added indent-off and indent-on tags
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 · 2020-05-20T22:57:40Z

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

matthijskooijman · 2020-05-21T05:35:22Z

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).

matthijskooijman · 2020-05-21T05:37:28Z

app/src/cc/arduino/packages/formatter/AStyle.java

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


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.

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

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.

matthijskooijman · 2020-05-21T05:39:24Z

app/src/cc/arduino/packages/formatter/AStyle.java

+        formattedText = formattedText.replaceAll(Pattern.quote(FORMAT_ON), "");
+
+        textArea.setText(formattedText);
+        textArea.select(selStart, selStart);


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).

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.

No negative point for usability

matthijskooijman · 2020-05-21T05:40:47Z

app/src/cc/arduino/packages/formatter/AStyle.java

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


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.

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

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 · 2020-05-21T14:43:39Z

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 ... =[

matthijskooijman · 2020-05-21T14:55:51Z

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:

Format the code before the selection. Save the both the original and the result, but do not apply it.
Format the code from the start of the file to the end of the selection.
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 · 2020-05-21T20:26:00Z

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:

I also added new test cases for selection formatting

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

matthijskooijman · 2020-05-22T08:08:03Z

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 · 2020-05-22T21:05:56Z

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 · 2020-05-24T10:39:53Z

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 · 2021-04-09T12:57:20Z

All committers have signed the CLA.

[editor] Allow formatting only the selection

2ba0124

matthijskooijman approved these changes May 14, 2020

View reviewed changes

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

fix code formatting

7efb118

format selection using AStyle control tags

cf81fdc

matthijskooijman reviewed May 21, 2020

View reviewed changes

Avoid erros on format multi-line comments, add Tests

0918a5b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[editor] Allow formatting only the selection #10204

[editor] Allow formatting only the selection #10204

ricardojlrufino commented May 14, 2020

matthijskooijman left a comment

ricardojlrufino commented May 14, 2020 •

edited

Loading

cmaglie commented May 15, 2020

ricardojlrufino commented May 15, 2020

ricardojlrufino commented May 15, 2020

cmaglie commented May 19, 2020

ricardojlrufino commented May 19, 2020

cmaglie commented May 20, 2020

ricardojlrufino commented May 20, 2020

matthijskooijman commented May 21, 2020

matthijskooijman May 21, 2020

ricardojlrufino May 21, 2020

matthijskooijman May 21, 2020

matthijskooijman May 21, 2020

ricardojlrufino May 21, 2020

ricardojlrufino May 21, 2020

matthijskooijman May 21, 2020

ricardojlrufino May 21, 2020

matthijskooijman May 21, 2020

ricardojlrufino commented May 21, 2020

matthijskooijman commented May 21, 2020

ricardojlrufino commented May 21, 2020

matthijskooijman commented May 22, 2020

ricardojlrufino commented May 22, 2020

matthijskooijman commented May 24, 2020

CLAassistant commented Apr 9, 2021 •

edited

Loading

[editor] Allow formatting only the selection #10204

Are you sure you want to change the base?

[editor] Allow formatting only the selection #10204

Conversation

ricardojlrufino commented May 14, 2020

All Submissions:

matthijskooijman left a comment

Choose a reason for hiding this comment

ricardojlrufino commented May 14, 2020 • edited Loading

About Eclipse formatter

cmaglie commented May 15, 2020

ricardojlrufino commented May 15, 2020

ricardojlrufino commented May 15, 2020

Before

After:

cmaglie commented May 19, 2020

ricardojlrufino commented May 19, 2020

cmaglie commented May 20, 2020

ricardojlrufino commented May 20, 2020

matthijskooijman commented May 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricardojlrufino commented May 21, 2020

matthijskooijman commented May 21, 2020

ricardojlrufino commented May 21, 2020

matthijskooijman commented May 22, 2020

ricardojlrufino commented May 22, 2020

matthijskooijman commented May 24, 2020

CLAassistant commented Apr 9, 2021 • edited Loading

ricardojlrufino commented May 14, 2020 •

edited

Loading

CLAassistant commented Apr 9, 2021 •

edited

Loading