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

added custom modes #59

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

Conversation

RichardHooijmaijers
Copy link

No description provided.

@vnijs
Copy link
Collaborator

vnijs commented Sep 21, 2018

@RichardHooijmaijers Can you provide a bit more information about how you envision this would work? Can you add a reproducible example as well?

@GregorDeCillia
Copy link
Contributor

GregorDeCillia commented Sep 25, 2018

Hello there. I just stumbled upon this PR and wanted to note something. It is generally not a good idea to copy files into the location where a package is built because of writing permissions. If an administrator builds the shinyAce package for all users of a Linux server, this feature will only be available for those with root permissions.

It would be a better idea to copy the JavaScript files into the sessions temporary directory (tempdir()) and tell aceEditor to load the resources from there. This can be archived with shiny::addResourcePath.

@vnijs
Copy link
Collaborator

vnijs commented Sep 26, 2018

Thanks for the PR @RichardHooijmaijers but I would have to agree with @GregorDeCillia on this one. Is there a reason you would not want to include the custom mode in shinyAce? Is it very large? Not generally useful outside of your application?

Perhaps another option might be to "source" the mode file by providing a full path to the mode file in your package using something like system.file?

@RichardHooijmaijers
Copy link
Author

The tempdir() / addResourcePath proposed by @GregorDeCillia indeed sounds very reasonable. I think it is possible to adapt the PR but this would probably mean that all modes needs to be available in the tempdir and I don't know if this is feasible or would cause any problems elsewhere. Could you please indicate if this is a viable option with regards to other coding in the package?

The way the package is setup, I don't think sourcing it would be an option, also the mode that was created is not really useful outside the application. Furthermore, I thought it would be nice to be able to include any user created mode.

@vnijs
Copy link
Collaborator

vnijs commented Sep 29, 2018

@RichardHooijmaijers I agree that it might be nice to be able to include any user created mode. However, it is not feasible to have all modes available a tempdir. If you can add an option to source a mode from another package or resource that could be a useful PR

@GregorDeCillia
Copy link
Contributor

GregorDeCillia commented Oct 2, 2018

The way I imagined it was to only move user-defined mode(s) into the tempdir when the aceEditor object is initialized but load all the default-modes form the shinyAce package directly.

The option to load modes only from packages seems a little bit rigid to me, since every change in the user-mode would require a new installation of the package containing it. Also, some users might want to add custom-modes without a package which would not be possible in that case.

Of course another option would be to create a hidden directory in home, for example ~/.shinyAce, where all the user-modes are stored. However, this would violate the CRAN policies but might still be implemented in a github-only branch.

Packages should not write in the user’s home filespace (including clipboards), nor anywhere else on the file system apart from the R session’s temporary directory

@vnijs
Copy link
Collaborator

vnijs commented Oct 2, 2018

If the user can specify a path to a mode to load that seems fine to me. That path could be in the tempdir or a package directory

@RichardHooijmaijers
Copy link
Author

I am not able to get this working unfortunately..
It seems that it is possible to only specify one location for the modes. Even though I could place the custom mode in the tempdir and adjust the "setMode" accordingly, it is not picked-up correctly. I think it might have something to do with "shinyAce.js" but could not figure it out in time.

If anyone want to give it a try, that would be great. For now I am gonna leave it as it is and explore other options..

@sbihorel
Copy link

sbihorel commented Jan 3, 2019

Hi @vnijs

I am interested in this functionality for an app that would require a single custom mode (and thus not part of the default list of modes). I am confused by the conflicted information of this thread and was wondering if you confirm if there is a way to implement this or not?

Thanks

PS: very cool and useful package by the way!

@vnijs
Copy link
Collaborator

vnijs commented Jan 3, 2019

@sbihorel Thanks for your interest @sbihorel. I liked @GregorDeCillia's suggestions on the setup but I don't know if it is feasible. If you can make it work, feel free to create a PR.

@GregorDeCillia
Copy link
Contributor

I can have a look into this over the weekend. From a conceptual point of view, this seems like a straight-forward feature to add but time will tell.

The way I imagined it would be to supply the mode in the constructor

shinyAce::aceEditor(
   mode = shinyAce::custom_mode("relative/or/absolute/path/to/mode.js")
)

or the update function.

shinyAce::updateAceEditor(
  session, editorId,
  mode = shinyAce::custom_mode("relative/or/absolute/path/to/mode.js")
)

The function custom_mode would basically just add a flag to the string so shinyAce knows wheter a predefined mode or a custom mode is requested. It seems this would also solve #24 to some capacity.

@sbihorel
Copy link

sbihorel commented Jan 3, 2019

@GregorDeCillia
That would be awesome!

@vnijs
Copy link
Collaborator

vnijs commented Jan 3, 2019

Very interesting indeed. I'm very curious to hear more about how this might address #24

@GregorDeCillia
Copy link
Contributor

GregorDeCillia commented Jan 6, 2019

Unfortunately I couldn't get this to work properly but it should be possible to change the resource folder for modes on the JavaScript side with

ace.config.set("modePath", ...)

as described in ajaxorg/ace#1518. This could potentially be combined with shiny::addResourcePath to make custom modes available. Basically, the current logic for resources works like this (assume a loopback interface with port 7257)

  • Add a resourcePath for shiny in R/init.R that links http://127.0.0.1:7257/shinyAce to the package directory
  • call something like editor.getSeession().setMode("ace/mode/r") in R/ace-editor.R.
  • setMethod internally uses the modePath registered for JavaScript which is http://127.0.0.1:7257/shinyAce/ace to load the js files from the package directory

I can't say if I will have time to dig into this deeper but I will try

@sbihorel
Copy link

sbihorel commented Mar 6, 2019

Hi,

A colleague of mine has created a custom mode file (mode-whatever.js) which works when included in ace editors inserted in regular web pages (ie, non shiny-based). I have tried to insert this file in the inst/www/ace folder of the shinyAce source, rebuilt the package and re-installed it. Now, when I run the 01-basic example app, the whatever mode is listed in the drop-down for modes but it does not seem to affect the editor content at all when I select this mode.
So my questions are the following:

  • if we choose the path of modifying the source package, do we need to do anything else to make this mode operational?, OR
  • does it mean that the mode file is buggy and do I need to tell my colleague to look into this more, maybe by comparing the file to other mode files provided in the shinyAce package?

Thanks

@vnijs
Copy link
Collaborator

vnijs commented Mar 7, 2019

@sbihorel I would indeed expect this to work. Check the JS console for any errors and compare to the other "mode" files

@RichardHooijmaijers
Copy link
Author

@sbihorel you can find an example for a custom mode in the issue related to this PR #57 (there is a zip attached at the end).

Although I really agree with @GregorDeCillia regarding not copying files to the installation directory of a package, if you want to test or use it in a controlled environment the following worked for me without adapting the original package:
file.copy("customMode.js"),paste0(system.file('www', package='shinyAce'),"/ace/customMode.js"))
Once the mode is available in the package you can directly apply it in your editor

@sbihorel
Copy link

@vnijs @RichardHooijmaijers
Yes, after comparison with other mode files, it was clear that the file created by my colleague had to be modified to work with shinyAce.
After that, a simple copy of the file inside the installation folder of shinyAce (actually in the www/ace subfolder) did the trick.
Thanks for the guidance.

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