Integrate with beautysh to provide auto-formatting

[f18m]

Hi,
I'm a user of BashEditor and I love it, thanks for creating this plugin.

However I think a useful feature is missing: what I would like to have in Eclipse/BashEditor is to have an option to run a Bash formatter every time I save the document (I'm also using CppStyle extension that allows you to run clang-format to reformat your C/C++ code everytime you click save).
I found over time that this is really the way to go to ensure consistent formatting of your code.
I think a good formatter is:
https://github.com/bemeurer/beautysh
so I propose to add an option in BashEditor to run that tool every time you click save.
What do you think?
I think I could help with this development if that's not too difficult !

Thanks,
Francesco

NOTE: my use case for automatic bash formatting is the following:
I have an annoying problem when using Eclipse to write Bash code: Eclipse by default uses TABs and not spaces to format it. This means that if I copy/paste a snippet of my bash into a terminal, it won't work (TABs are recognized as line-completion triggers by the terminal). What I do is to copy/paste the code into another editor, replace TABs with spaces and then copy/paste to the terminal. This is very slow and error-prone. With auto-reformatting that problem would be automatically solved with a simple CTRL+S

[de-jcup]

Hello Francesco,

first of all: I think adding a save action is a very good idea!

About your problem with tabs:

Bash editor uses default text editor settings which can be set to use always spaces:

This will use always spaces instead of tabs and maybe could be already a solution for your use case.

About help in development

I appreciate your hep. Please look at https://github.com/de-jcup/eclipse-bash-editor/wiki/Contribution for some wanted settings to keep things easier.

There is a branch called feature-117. Please checkout.
There already some things done (copied from egradle and adopted).
TODO:

1. ExternalToolCommandArrayBuilderTest does currently fail and ExternalToolCommandArrayBuilder must be implemented.
2. The main preference page of editor (BashEditorPreferencePage) must be extended to have an
   optional string field entry for the command line to execute (+ hint about $filename)
3. On save the editor must check if the preferneces are enabled and call SimpleProcessExecutor with
   BashEditorFileProcessContext and results from ExternalToolCommandArrayBuilder
4. If external tool was used, the editor must reload content after execution and set it as editor content
5. Common Error handling
   • file not existing
   • timeout (should be already done in simple process executor)

[f18m]

Hi @de-jcup ,
Thanks for pointing me to a possible workaround... however I need this kind of re-formatting to be done just for Bash scripts.
I did checkout the feature-117 branch and applied git settings from wiki page.
However I'm still in the learning curve - I used Maven for my Java projects in the past and I need to read a bit about Gradle first.
I will see what I can do ! Thanks in the meanwhile for pointers!

[de-jcup]

Hello @f18m,

I updated https://github.com/de-jcup/eclipse-bash-editor/wiki/Contribution and give more information about initializing, developing bash editor and also about necessary usage of gradle.

Please read the new information. In a nutshell: For development you need a Eclipse RCP edition. Full build is only possible by Eclipse. Gradle does only build and test eclipse independent parts.

[f18m]

Hi @de-jcup ,
thanks for the info. I read the wiki page and I'm reading the article at
http://www.vogella.com/tutorials/EclipsePlugin/article.html

I downloaded Eclipse for RCP and RAP developers version 2018-12, I did implement the ExternalToolCommandArrayBuilder (in a dummy way for now - just to get the test passed). I did run the gradle wrapper with build and eclipse tasks (both successful).
I did import the project as Existing Gradle project in Eclipse and created a launch configuration as "Eclipse Application". However when I try to run it I get the following validation errors:

I tried to go in the launch configuration page, "Plug-ins" tab and use "launch with: plugins selected below only" and click "Add required plug-ins" but that did not solve the issue.
I tried googling the problem and the best match is probably
https://stackoverflow.com/questions/18420283/launching-plugin-fails-missing-constraint-javax-xml-bind
but I did not find any working solution for me.
Are you using a different Eclipse version (like 2018-09) ?

[ UPDATE: apparently this must be some kind of version conflict inside Eclipse included packages because it's totally unrelated to BashEditor plugin: I get these errors also when I create the "Hello World" Eclipse plugin from scratch - and these errors do not seem to impact on the plugin functionalities so I'm going to just ignore them for now! ]

If I click "continue" when launching the Eclipse application I then have problems when I try to view the Bash Editor preferences page:

This is the real blocking problem for me now. I understand that, as written in the wiki page, the gradle build system will not build the src/main/java-eclipse folder where that preference page is actually located. However it's unclear to me what it means that "Currently the build is done in oldschool way which means the Eclipse RCP tools are used to build the plugin site."
I tried importing a fresh git checkout of the plugin as "Plug-in Development->Plug-ins and fragments" - in that way I think Eclipse is just compiling and building whatever Java source it finds (indeed it compiles also the BashEditorPreferencePage.java source file!). However I still cannot go past the "Unable to create the selected preference page. de.jcup.basheditor.preferences.BashEditorPreferencePage cannot be found by de.jcup.basheditor_1.5.3" error.
It's like that source file is missing from the JAR created by the build...

Maybe you have some hints to share...

Thanks

[de-jcup]

Hello @f18m ,

I think this is an JDK 11 issue.

I updated contribution wiki page and added link to video at youtube: https://youtu.be/9_AQouAvehY

At the video I checked out the project complete fresh from github and used a clean workspace and also a clean Eclipse RCP 2018-12 (linux).

I started eclipse with Open JDK 10, launching was done with JDK11. Doing this I got same warnings as you, but I was able to start the bash editor plugin without problems.

Maybe the video gives you all information you need. I did not called the gradlew eclipse parts, because only "basheditor-other" project is created by this way is not necessary for startup. But calling the gradle tasks does no changes, so I removed it from the video.

So my suggestion: Get a JDK 10 and copy it to a location to your system. Then change the eclipse startup and define in eclipse.ini to use JDK 10 as JVM, for details look at https://wiki.eclipse.org
/Eclipse.ini

I opened a bug at https://bugs.eclipse.org/bugs/show_bug.cgi?id=543198 because every developer using JDK 11 will have such problems.

[f18m]

Hi,
thanks for all the details. Indeed the issue was due to the use of
/usr/lib/jvm/java-8-openjdk-amd64/jre/bin/java
as JVM.

I installed the "java-11-openjdk-amd64" package (I'm on Ubuntu 18.04.1 LTS), switched Eclipse to use that (editing eclipse.ini) and imported the project in a clean workspace just as you did and now it works! The Bash Editor preference page is displayed just fine.
For some reason even if the package is named "java-11-openjdk-amd64" package, the version it reports is "openjdk 10.0.2 2018-07-17".
Perhaps in the wiki page it would be useful to suggest using JDK 10 at least, since for some reason JDK 8 gives troubles...

Thanks for the help, I now think can proceed with actual implementation of the feature :)

[f18m]

Hi @de-jcup
Regarding the TODO list you wrote down I did some basic work on my forked feature-117 branch at:
https://github.com/f18m/eclipse-bash-editor

TODO:

1. ExternalToolCommandArrayBuilderTest does currently fail and ExternalToolCommandArrayBuilder must be implemented.

I did a very simple implementation that tokenize the given string and detects the special placeholder $filename.

2. The main preference page of editor (BashEditorPreferencePage) must be extended to have an
   optional string field entry for the command line to execute (+ hint about $filename)

Done (no hint for now - I can look into that later)

3. On save the editor must check if the preferneces are enabled and call SimpleProcessExecutor with
   BashEditorFileProcessContext and results from ExternalToolCommandArrayBuilder

I'm thinking how to exactly do this: for example, another Eclipse extension that does something similar is CppStyle (allows to run clang-format on every save) and it's implementing this save action as:
https://github.com/wangzw/CppStyle/blob/master/plugin/src/org/wangzw/plugin/cppstyle/ui/CppStyleHandler.java

i.e. creating a derived class from AbstractSaveHandler.
However BashEditor already extends AbstractTextEditor eclipse class that contains the overrideable method performSave()... that could be another way to implement the external tool integration.
Yet another possibility I see would be to place such integration into the FileDocumentProvider-derived classes like BashFileDocumentProvider...

what do you think is the most correct way?

Thanks

[f18m]

Hi @de-jcup

I'm thinking how to exactly do this: for example, another Eclipse extension that does something similar is CppStyle (allows to run clang-format on every save) and it's implementing this save action as:
https://github.com/wangzw/CppStyle/blob/master/plugin/src/org/wangzw/plugin/cppstyle/ui/CppStyleHandler.java

i.e. creating a derived class from AbstractSaveHandler.
However BashEditor already extends AbstractTextEditor eclipse class that contains the overrideable method performSave()... that could be another way to implement the external tool integration.
Yet another possibility I see would be to place such integration into the FileDocumentProvider-derived classes like BashFileDocumentProvider...

what do you think is the most correct way?

Actually I tried using the CppStyle approach but the AbstractSaveHandler implementation I added was never being called (I added also an extension hook in plugin.xml but still did not work).
So I went for the BashFileDocumentProvider solution and it worked!

I did a very simple proof of concept implementation (needs a lot of cleanup) in my forked repo...
I will let you know when I think it's ready for review!

[de-jcup]

Hello @f18m ,

I would always override editor method. This is the first place any other developer would look into and its a central point.

I would do following:

• override the performSave method.
• use text entry from editor preference page and push it into array builder and use the result
  in SimpleProcessExecutor. Execution should be done in a blocking eclipse job later so it will not freeze
  the UI...
  After execution is done the converted text (bash) file must be loaded again and set in text editor document

Hope it helps.

[f18m]

I would always override editor method. This is the first place any other developer would look into and its a central point.

ok I moved the code to the performSave() method

• override the performSave method.

ok, done

• use text entry from editor preference page and push it into array builder and use the result
  in SimpleProcessExecutor. Execution should be done in a blocking eclipse job later so it will not freeze the UI...

ok, done

After execution is done the converted text (bash) file must be loaded again and set in text editor document

ok, done but I have spent some time trying to restore correctly the caret position to the position it had before reloading contents... I managed to do it even if the way I did it is not perhaps the best one / most clean way to do it.

[f18m]

PS: I pushed all my changes to my fork: https://github.com/f18m/eclipse-bash-editor
(I realized yesterday I did commit but not pushed the changes to GIT!)
if you want to take a look

[de-jcup]

@f18m : I just merged you last PR #125 and tested out with using

I tried out with beautysh -f $filename and it works well 👍 Great Job!

I thinks its mostly done now.

If you like I would do some final steps:

1. Move the save actions to a dedicated sub page
2. Change the Note: to a tooltip and set beautysh -f $filename as default external tool operation for reformatting.
3. I would do some auto code formatting (java code style instead of c/c++ style) and adding some brackets

if (true) {
    println "its true"
}

instead

if (true)
    println "its true"

4. Will update documentation

Would this be okay for you or do you want to do more by yourself?

[f18m]

Hi Albert,
Sound just fine thanks!!

Looking forward for the next release then!! :)

[de-jcup]

Hi Francesco,
sorrowly I found a bug while changing already preferences page and testing output.
It seems the refreshLocal() does not always work. I often got the problem that the external tool changes the file but the save action does not load it - i loaded it parallel with a text editor and the changes were done. But I will try to fix it.

[de-jcup]

I found the problem:

1. super.performSave() - changes the file on file system (timestamp with max precission: seconds...)
2. call external program (too fast... sometimes same second... so timestamp does not change...
3. refresh local

Tried out:

1. super.performSave() - changes the file on file system (timestamp with max precission: seconds...)
   1b) wait 1 second
2. call external program (too fast... sometimes same second... so timestamp does not change...
3. refresh local

seems to works always.

Of course there should be a better solution. Maybe a) temp files or b) force reload , ...

[de-jcup]

[f18m]

Hi @de-jcup ,

I found the problem:

1. super.performSave() - changes the file on file system (timestamp with max precission: seconds...)
2. call external program (too fast... sometimes same second... so timestamp does not change...
3. refresh local

wow, such a low timestamp precision looks really like a strong assumption from Eclipse developers...
good finding!

Tried out:

1. super.performSave() - changes the file on file system (timestamp with max precission: seconds...)
   1b) wait 1 second
2. call external program (too fast... sometimes same second... so timestamp does not change...
3. refresh local

seems to works always.

Of course there should be a better solution. Maybe a) temp files or b) force reload , ...

I tried latest branch feature-117...
honestly however the sleep 1 second is quite noticeable and makes it feel a bit slow.
I wonder if it makes more sense to go back to a previous attempt where I did call

• super.performSave() - changes the file on file system
• getDocument().set(external_tool_result);
• reset caret position
• super.performSave() again to remove the "dirty" status

See https://github.com/f18m/eclipse-bash-editor/blob/c69eb8e6fba7313ac7bfe6fb36dc05f4565b98d5/basheditor-plugin/src/main/java-eclipse/de/jcup/basheditor/BashEditor.java

[f18m]

Just another feedback on the new preference page: looks nice!
However it's still not resizeable in horizontal and e.g. I see the "Note" text field truncated.
Moreover if I put a long path for beautysh it will be truncated as well.

In addition it probably makes sense to:

• validate the existence of the process specified in the textbox
• disable the textbox if the checkbox is not ticked

(I can try to help with these if you think they make sense)

[f18m]

Another thing: beautysh by default will use spaces for Bash files (which make more sense than TABs) but I see that in the file BashSourceViewerConfiguration.java the text viewer is using the

fPreferenceStore.getBoolean(AbstractDecoratedTextEditorPreferenceConstants.EDITOR_SPACES_FOR_TABS)

to understand whether using spaces or TABs... maybe it makes sense to add a checkbox for BashEditor to allow it to override the generic setting of Eclipse about TABs / spaces?

(PyDev extension has such kind of settings under PyDev->Editor->Tabs which explicits mention that PyDev ignores the "insert spaces for tabs" in the general settings page and will instead apply the Python-specific settings)

[de-jcup]

I just created #127 for your idea about overriding standard text editor preferences. If you have more ideas or want to contribute for your wanted feature please use this issue.

[de-jcup]

I tried latest branch feature-117...
honestly however the sleep 1 second is quite noticeable and makes it feel a bit slow.
I wonder if it makes more sense to go back to a previous attempt where I did call

The "one second" timestamp diff is the safe way when using refreshLocal approach.

If you get your former approach smooth working without the delay please try to implement it. But it must work always. And please let the execution still be done in the canceable job. So a long running operation can be canceled by user and a job does also handle the coreexception and show a detail screen to user on problems.

Please do the PRs as before to the feature branch

[de-jcup]

About the preference page suggestions:

• using standard field editors without real customized SWT components is extreme tricky and not easy
  Normally I would use no StringFieldEditor but a SWT Text component directly and would use MULTI and scrollable to get better editor possibilities. But this comes up with many problems when mixing field editors with "normal" components! I did this before and it was oftern very tricky.
• validate existance: this would be an additional button and is not easy to implement with normal field editors
• disable when not ticked: yes nice, but with some effort.

You can look into
https://github.com/de-jcup/egradle/blob/master/egradle-plugin-ide/src/main/java-eclipse/de/jcup/egradle/eclipse/ide/ui/RootProjectConfigUIDelegate.java for inspiration
where I did all this for EGradle before (the delegate is used by an importer and the gradle preference page)

Feel free to implement the wanted behaviours if you like. But be aware: The preference page must be tested very well because doing very customized preference pages will often result in unexpected side effects (e.g. allowing now empty entries can block the preference page - so user cannot leave, or for e.g. handling of initializing of current enabling etc.

[f18m]

Hi @de-jcup

I tried latest branch feature-117...
honestly however the sleep 1 second is quite noticeable and makes it feel a bit slow.
I wonder if it makes more sense to go back to a previous attempt where I did call

The "one second" timestamp diff is the safe way when using refreshLocal approach.

If you get your former approach smooth working without the delay please try to implement it. But it must work always. And please let the execution still be done in the canceable job. So a long running operation can be canceled by user and a job does also handle the coreexception and show a detail screen to user on problems.

Please do the PRs as before to the feature branch

well I think that this issue (bypassing refreshLocal) would be more like an optimization; I think that your current code is already doing a good job!

About the preference page suggestions:
using standard field editors without real customized SWT components is extreme tricky and not easy
Normally I would use no StringFieldEditor but a SWT Text component directly and would use MULTI and scrollable to get better editor possibilities. But this comes up with many problems when mixing field editors with "normal" components! I did this before and it was oftern very tricky.
validate existance: this would be an additional button and is not easy to implement with normal field editors
disable when not ticked: yes nice, but with some effort.

I worked on that a little bit, see #128
As you mentioned probably the "validate everything is ok" is a bit of effort so I preferred copying the CppStyle approach where they used a FileFieldEditor to validate at least the existence of the external tool...

I just created #127 for your idea about overriding standard text editor preferences. If you have more ideas or want to contribute for your wanted feature please use this issue.

I will try to approach this now!

[de-jcup]

Hello @f18m . Thank you. I put some review marks into PR.
About "validate everything is okay". I wrote some lines into PR about the FileFieldEditor approach - I am not s happy about having explicit pathes insidethe defaults etc. So it does only work out of the box for linux apt installs.
I would prefer to use Stringfieldeditor , but would validate (either by extra button or with standard validate perference methods) by simply creating a temporary bash file and execute - result 0 means okay. This will work on every OS. And if the tool is not inside path, users have still the choice to either change path or set file path explicit.

[de-jcup]

@f18m There is still an issue:

When we got code which has validation errors, the formatter will also be called.
This should not happen.

We must check on isRunningExternalToolOnSave() also if the current model has got errors inside. If so the external tool should not called - because we got additional errors from external tools and may have also corrupted error markers etc.

I will fix this after I got your final PR merged .

[f18m]

Hello @f18m . Thank you. I put some review marks into PR.

Hi @de-jcup I cannot see the review marks in the PR...
I usually see them right in the page of the PR... #128
am I missing something?

About "validate everything is okay". I wrote some lines into PR about the FileFieldEditor approach - I am not s happy about having explicit pathes insidethe defaults etc. So it does only work out of the box for linux apt installs.

yes, you're right - I didn't consider this aspect. I basically use Linux only :)

I would prefer to use Stringfieldeditor , but would validate (either by extra button or with standard validate perference methods) by simply creating a temporary bash file and execute - result 0 means okay. This will work on every OS. And if the tool is not inside path, users have still the choice to either change path or set file path explicit.

yes, that's another approach. Is it possible to create a "normal" SWT button in a preference page using field editors or the extra "validate" button should be a fake field editor?

[f18m]

I would prefer to use Stringfieldeditor , but would validate (either by extra button or with standard validate perference methods) by simply creating a temporary bash file and execute - result 0 means okay. This will work on every OS. And if the tool is not inside path, users have still the choice to either change path or set file path explicit.

yes, that's another approach. Is it possible to create a "normal" SWT button in a preference page using field editors or the extra "validate" button should be a fake field editor?

apparently it's possible without too much troubles... I just checked in this approach in my PR branch.
Please take a look again at #128

[de-jcup]

I haved merged your PR to remote branch + made some changes.
Have you still some ongoing changes? If not then I would do some final cleanup and close the issue + merge to master

[f18m]

no I don't have any more ongoing changes - at least for this "Save action" thing, thanks!
Looking forward for next release of BashEditor with this feature then :)

[de-jcup]

Will apply the feature in next days to master and release will follow.
Thanks for your idea, work and patience.

[f18m]

Ok, then I'm closing this.
Keep up the great work!

[de-jcup]

@f18m I just released Version 1.6.0 containing the feature

[f18m]

Hi @de-jcup,
I've taken the new release and I'm using it together with beautysh v4.1 with external tool command line:

beautysh -f $filename --force-function-style=fnpar

It works very well, thanks!!

[de-jcup]

Update: Here a snapshot how the preferences looks initial when Bash editor 2.9.0 is fresh installed: