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

Numerous improvements to rlang autocomplete, tooltips & annotations #66

Merged
merged 38 commits into from
Sep 5, 2019

Conversation

dgkf
Copy link
Contributor

@dgkf dgkf commented Aug 30, 2019

In an effort to bring shinyAce closer to expected behaviors of an IDE, I've made quite a few changes to the rlang autocompleter, added help dialog tooltips, gutter annotations for syntax errors and additional keybinds.

shinyAce rlang updates

rlang autocomplete

I drew inspiration from the learnr package's autocomplete code (which has its own, non-shiny wrapper around Ace) to take advantage of some autocomplete edge cases that are better handled in that implementation.

image

image

Feature updates:

  • functions now autocomplete with trailing (), inserting cursor between the parenthesis
  • packages now autocomplete with trailing ::
  • autocomplete suggestions indicate in the meta field which package the objects are exported from
  • within a function call, autocomplete will show available formal arguments, with default values shown if available.
  • added ability to trigger autocomplete following . and : characters

rlang tooltips

To bring this autocompletion more in line with expected behaviors from the RStudio IDE, lingering on an autocomplete option will also show the help dialogue, rendered as html-formatted markdown.

  • lingering on a package name will show the package documentation
  • lingering on a function argument will show section for that particular argument extracted from the function documentation

image

rlang annotations

To improve immediate feedback about code issues, I also added gutter annotations for syntax errors. Upon update, after a small delay, the annotator will attempt to parse the full block of code and return annotation information back to Ace.

image

Hotkey Improvements

Tab completion was added, trying to intelligently kick off a new autocompletion, select available completions or indent the current code. This behavior is meant to mimic the RStudio IDE more closely.

General shinyAce Updates

  • Improved the ability to embed shinyAce in a shiny module
  • Added example for embedding in module
  • Added example of using rlang annotator and tooltips

Known Bugs

It was my intention to make this pull request quite a while ago and was just recently reminded of the work. Seeing that I was already accruing merge conflicts, I wanted to open the pull request before trying to resolve any known issues so that you're aware of this work. That said, there are still some known minor bugs.

  1. Since the help tooltip appears only after a slight delay, I've occasionally encountered situations where I will tab complete the code and the help tooltip appears after already selecting a completion, appearing off to the left of the code block.
  2. Tab completion of functions will occasionally add a closing parenthesis twice. data.fr_ <TAB> data.frame(_)). I think this happens when I hit tab twice, but I haven't dug into it to be sure.

dgkf added 24 commits May 24, 2019 07:55
improvements to rlang completer, tooltips, annotations, hotkeys
@dgkf dgkf marked this pull request as ready for review August 30, 2019 16:42
@dgkf
Copy link
Contributor Author

dgkf commented Aug 30, 2019

Just a quick note, there is some Javascript that is added to handle the tooltip callbacks and the rlang completion insertion behavior. Right now I believe it would be applied regardless of completer, so it might be necessary to retool it a bit to that it only enables that behavior when the rlang completer is used. My Javascript is pretty shaky, so I really just got it working before trying to engineer a more language agnostic solution.

There are two behaviors that are applied generally, and should probably only affect the R language:

  1. When inserting a completion, it will clobber existing text, i.e.

    # inserting a tab completion with the cursor in the middle of a word
    su|m
    # | sum
    # |>summarry
    
    # will clobber the remainder of the valid R object name
    summary(|)

    https://github.com/dgkf/shinyAce/blob/fe7468ffc39e3a38137ca6f8b9d97fc52c4151f8/inst/www/shinyAce.js#L273-L322

  2. Tab completion decides when to insert a tab or trigger autocomplete based on whether the cursor is in an R function call or object name

    https://github.com/dgkf/shinyAce/blob/fe7468ffc39e3a38137ca6f8b9d97fc52c4151f8/inst/www/shinyAce.js#L376-L407

@dgkf
Copy link
Contributor Author

dgkf commented Aug 31, 2019

I assume you meant [...] to only work on codeblocks

Right you are, edited my comment

Could you revert this part to the previous approach?

I'm unclear on which part you'd like to revert. I think it already works the way you're expecting. To enable each of the new features, you must create a new observer (from tooltip example):

  # Enable/Disable R code completion / annotation
  ace_completer <- aceAutocomplete("ace_editor")
  ace_annotator <- aceAnnotate("ace_editor")
  ace_tooltip   <- aceTooltip("ace_editor")

To disable the annotator or tooltips, you'd just remove that line which creates the observer, or suspend the observation, just like you already would with the autocompleter:

observe({
if (input$enableRCompletion) {
mutateOb$resume()
plotOb$resume()
} else {
mutateOb$suspend()
plotOb$suspend()
}
})

Perhaps I'm misunderstanding. I'm happy to refactor to fit your needs, I'm just unclear of the goal.

@vnijs
Copy link
Collaborator

vnijs commented Aug 31, 2019

Thanks for the clarification @dgkf. What I meant in my previous comment was to revert the change that requires rlang be selected for tooltips and annotater to work. I think it was this commit d513583. Basically, tooltips and annotater should work with example 07-autocomplete-combine even if line 35 in server.R is set to the below (i.e., if "rlang" is removed).

autoCompleters = c("static", "text")

@dgkf
Copy link
Contributor Author

dgkf commented Sep 1, 2019

I see, thanks for clarifying @vnijs.

I moved the .getDocTooltips initialization (where the tooltips Shiny callback is added) to the update section so that it gets bound to all selected completers. Now even without "rlang" selected as a completer, the tooltip callback will still get executed if the observer is running. The annotations were already working for all completers, so now both should be working across the board.

@vnijs
Copy link
Collaborator

vnijs commented Sep 1, 2019

Thanks @dgkf but I'm not getting this to work. After changing datasets, the below works only if "rlang" is added as a completer. Could you take a look? FYI this is the adapted 07 example I mentioned earlier.

library(shiny)
library(shinyAce)
library(dplyr)

ui <- fluidPage(
  titlePanel("shinyAce auto completion - combine completion lists"),
  radioButtons("dataset", "Dataset: ", c("mtcars", "airquality"), inline = TRUE),
  uiOutput("ace_editor")
)

server <- function(input, output, session) {
  
  ## Dataset Selection
  dataset <- reactive({
    get(input$dataset)
  })
  
  comps <- reactive({
    comps <- list()
    comps[[input$dataset]] <- colnames(dataset())
    comps <- c(comps, list(dplyr = getNamespaceExports("dplyr")))
  })
  
  output$ace_editor <- renderUI({
    ## initially, only show completions in 'comps' (i.e., dplyr and selected dataset)
    shinyAce::aceEditor(
      "editor",
      mode = "r",
      value = "select(wt, mpg)\n",
      height = "200px",
      autoComplete = "live",
      autoCompleters = "static",
      autoCompleteList = isolate(comps())
    )
  })
  
  ## Update static auto complete list according to dataset and add local completions
  observe({
    shinyAce::updateAceEditor(
      session,
      "editor",
      # autoCompleters = c("static", "text", "rlang"),
      autoCompleters = c("static", "text"),
      autoCompleteList = comps()
    )
  })
  
  ## adding an observer for R-language code completion
  ## will become active after the first switch to another
  ## dataset
  ace_completer <- aceAutocomplete("editor")
  ace_annotator <- aceAnnotate("editor")
  ace_tooltip   <- aceTooltip("editor")
}

shinyApp(ui = ui, server = server)

@dgkf
Copy link
Contributor Author

dgkf commented Sep 1, 2019

That code is working for me. Is it possible that you didn't rest on a completion item long enough for the tooltip to pop up? The tooltips are set up to only show after an item in the dropdown has been hovered over for 1s. This is to prevent some jankyness when switching completion fields (showing the last completion hover until the R process completes) and to minimize the overhead imposed on the R process for parsing help docs.

editor.__tooltipTimerCall = lang.delayedCall(
  function() { 
    if (this.completer.activated)
      this.completer.showDocTooltip(data.docTooltip); 
  }.bind(editor), 1000);

from shinyAce.js #L236-240

If you aren't already, make sure once the completions list pops up that you linger on an item in the dropdown list for that 1s delay to wait for the tooltip to show. The tooltip text for the dataset column names isn't particularly helpful and will just show the name of the column. For functions as shown in the screenshot it should pull up the documentation entry.

image

@vnijs
Copy link
Collaborator

vnijs commented Sep 1, 2019

@dgkf That part does indeed work. Thanks. I was looking at argument completion inside brackets that activates after pressing the tab key within brackets. That still only seems to work when "rlang" is set.

image

@dgkf
Copy link
Contributor Author

dgkf commented Sep 1, 2019

The argument completions are handled in the aceAutocompleter just like the rlang completions. As the package is now (without my changes), this callback reacts to the <inputId>_shinyAce_hint value updated in the rlangCompleter.

var rlangCompleter = {
getCompletions: function (editor, session, pos, prefix, callback) {
var inputId = editor.container.id;
// TODO: consider dropping onInputChange hook when completer is disabled for performance
Shiny.onInputChange(inputId + '_shinyAce_hint', {
// TODO: add an option to disable full document parsing for performance
document: session.getValue(),
linebuffer: session.getLine(pos.row),
cursorPosition: pos,
// nonce causes autocomplete event to trigger
// on R side even if Ctrl-Space is pressed twice
// with the same linebuffer and cursorPosition
nonce: Math.random()
});
// store callback for dynamic completion
$('#' + inputId).data('autoCompleteCallback', callback);
}
// TODO: add option to include optional getDocTooltip for suggestion context
};
langTools.addCompleter(rlangCompleter);

If it's preferred to have this behavior globally it would probably require a pretty extensive rewrite of the way the rlang completer is currently set up. I think this is probably beyond the scope of the additions I've made and might warrant a separate pull request.

@vnijs
Copy link
Collaborator

vnijs commented Sep 2, 2019

@dgkf I'm starting to figure out how to use your additions a bit better now. Thanks again for the clarification. If possible I'd still prefer to fully remove any changes related to modules from this PR and submit any changes you deem necessary for use of shinyAce with modules as a separate PR so @detule can review this in detail. That said, I'm very excited about what you have been able to do with autocompletions! These are substantial improvements indeed. Below some initial questions and comments that I think could help improve the setup further.

Is there a way to avoid an "=" sign when not needed? For example:

image

Is it possible to escape quotes in the argument list that? For example, "independent" should be shown with quotes as that is the default value. I noticed that Rstudio doesn't seem to show default values at all so perhaps this is tricker than it seems at first glance?

image

If you have autocompleters = ("text", "rlang") the completions offered when inside function brackets will contain all words in a document. Could autocompletions for function arguments drop any "local" text options provided through the "text" autocompleter?

image

In example 12-tooltips you have a checkbox to allow completion of variable names linked to a dataset. I assume this is supposed to work with "static" but there is no dataset and no custom completion list specified. Perhaps you could use "mtcars" as the example dataset and add "varnames(mtcars)" as the custom completion list?

image

@dgkf
Copy link
Contributor Author

dgkf commented Sep 2, 2019

@vnijs - Thanks for taking a close look at some of the edge cases. I took a look at the way I was building the tooltip info to try to make it a bit more consistent. I think the code is in a better place now. I was able to address most, but not all of your suggestions.

I'd still prefer to fully remove any changes related to modules from this PR

The only step taken for addressing this is that the inputId used for sending the custom message is wrapped in ns() - the shiny function for namespacing a module ID. This has no effect in cases where modules aren't used and would be a very minor concern if ever trying to merge other code. If there are sweeping changes being made to better handle modules, this change would be of almost no significance to the refactoring. Given that it would break these features when modules are used and comes at very little code overhead, I would much prefer to leave it in.

return(session$sendCustomMessage('shinyAce', list(
  id = session$ns(inputId), # only change was to add `session$ns()`
  codeCompletions = jsonlite::toJSON(completions, auto_unbox = TRUE)
)))

Is there a way to avoid an "=" sign when not needed?

Yes! Done - and now the package description is displayed for packages instead of just the name. In the same vein, I also added the ability to dig up dataset documentation (e.g. mtcar_)

Is it possible to escape quotes in the argument list that?

Yes! Good eye. I hadn't noticed that. I added checking for the edge case where the formal argument default is a character vector of length 1.

Could autocompletions for function arguments drop any "local" text options?

Possibly... I'd have to think on this one a bit. This is a bit tricky because it's handled at the Javascript level given the different completers that have been added. The two lists of completions (the arguments provided by the rlangCompleter and the local values provided by the textCompleter) are handled independently and then merged. I'm sure there's a way, but it would probably be quite challenging to get it working. I would consider this out of scope for a first pass at the argument autocompletion.

In example 12-tooltips you have a checkbox to allow completion of variable names

This was laziness on my part. I just copied another example as a quick testing grounds and didn't realize I never went back to clean it up. I reworked it to just showcase the tooltips and annotation features as an example of how to disable them reactively.

@vnijs
Copy link
Collaborator

vnijs commented Sep 4, 2019

Thanks for the updates @dgkf. These changes are great ... but there seems to be some corner cases related to tooltips. I'm seeing a lot of messages related to print(e$message) in ace-tooltip.R (e.g., length is 0 or length is more than 1 in if statements). Some of the labels also seem incorrect. For example (see below) where r_session is an environment an not a package resulting in Warning in utils::packageDescription(v$value, fields = c("Title", "Description")) : no package 'r_sessions' was found.

You might want to use length(v) == 0 instead of is.null(v) as the former also captures c().

image

In general, printing errors and warnings for autocompletion is great for debugging but should be shown in the final version of the upcoming release with your changes.

@vnijs
Copy link
Collaborator

vnijs commented Sep 4, 2019

Another common warning message from, I think, tooltips. FYI I'm using R 3.6.1 on a pretty new computer where I have only used R 3.4 and above.

image

@dgkf
Copy link
Contributor Author

dgkf commented Sep 4, 2019

Thanks again for your diligence on this front @vnijs

I've been able to reproduce the first issue trying to autocomplete .GlobalEn_. I think it's due to the way I'm testing for package namespaces and it being a bit ambiguous with environment variables specifically. I'll try to come up with something more robust and tackle the 0-length conditional error. I think these issues should be some low hanging fruit.

I'll follow up once I've fixed those cases.

@dgkf
Copy link
Contributor Author

dgkf commented Sep 4, 2019

I tried my best to address these issues but had troubles reproducing a couple of them.

no package 'r_sessions' was found

I don't have an r_sessions by default in my R instance, but I think this variable might be an environment, which was getting inappropriately interpreted to be a namespace and was therefore looking for package documentation. I think I have a more robust way of checking whether something is a package now and consider environments as separate data types.

You might want to use length(v) == 0 instead of is.null(v)

I think this was coming from the way the r_help_type tooltip metadata was being compared against possible values. It now uses %in% instead, which will still return a logical even if r_help_type is NULL

In general, printing errors and warnings for autocompletion is great for debugging but should [not?] be shown in the final version of the upcoming release with your changes

Agreed - I added a shinyAce_debug message to wrap debugging statements, which is only displayed if the option shinyAce.debug is set to TRUE. I also added some documentation under the name shinyAce-options to document this functionality.

image

I haven't been able to reproduce this error message. I think it might be related to packages that don't have documentation or treating environments as packages before. If you see this again, please document some steps to reproduce it. That would help me a lot for drilling into the problem.

@vnijs
Copy link
Collaborator

vnijs commented Sep 5, 2019

Thanks @dgkf. Looking much more solid already. I'm able to reproduce the error below using the shiny app code below. There are other examples where this happened but I expect this will cover most (or all) cases of this warning. You'll need to install "radiant.data". In the ace editor type visualize and then press tab with the cursor within the (). The full code, with arguments, for the visualize function is linked below.

https://github.com/radiant-rstats/radiant.data/blob/master/R/visualize.R

Warning in if (!file.exists(dirpath)) stop(gettextf("invalid %s argument",  :
  the condition has length > 1 and only the first element will be used
Warning in if (!file.exists(paste0(RdDB, ".rdx"))) stop(gettextf("package %s exists but was not installed under R >= 2.10.0 so help cannot be accessed",  :
  the condition has length > 1 and only the first element will be used

image

library(shiny)
library(shinyAce)
library(radiant.data)

ui <- fluidPage(
  titlePanel("shinyAce auto completion demo"),
  sidebarLayout(
    sidebarPanel(
      checkboxInput("enableAutocomplete", "Enable R AutoComplete", TRUE),
      checkboxInput("enableTooltips", "Enable R Tooltips", TRUE),
      checkboxInput("enableAnnotations", "Enable R Annotations", TRUE)),
    mainPanel(
      aceEditor("ace_editor", 
                mode = "r", 
                autoComplete = "live",
                autoCompleters = "rlang", 
                value = "# Tooltips:
# linger over an autocomplete option to view some documentation. See
# - Function descriptions
#     data.fra  # <autocomplete>
# - Argument descriptions
#     data.frame(  # <autocomplete>
# - Package descriptions
#     shinyAc  # <autocomplete>


# Annotations:
# gutter annotations used to indicate syntax errors, try uncommenting this line 
# with an incomplete string
# '''


"
      )))
)


server <- function(input, output, session) {
  # Enable/Disable R code completion / annotation
  ace_completer <- aceAutocomplete("ace_editor")
  ace_annotator <- aceAnnotate("ace_editor")
  ace_tooltip   <- aceTooltip("ace_editor")
  
  # Enabling and Disabling Autocompletion Observer  
  observe({
    if (input$enableAutocomplete) ace_completer$resume()
    else ace_completer$suspend()
  })
  
  # Enabling and Disabling Tooltips Observer  
  observe({
    if (input$enableTooltips) ace_tooltip$resume()
    else ace_tooltip$suspend()
  })
  
  # Enabling and Disabling Annotations Observer  
  observe({
    if (input$enableAnnotations) ace_annotator$resume()
    else ace_annotator$suspend()
  })
}

shinyApp(ui = ui, server = server)

@dgkf
Copy link
Contributor Author

dgkf commented Sep 5, 2019

Ah! This is perfect. It's because there are multiple documentation files for visualize (you can try ?visualize in the console and you get prompted with multiple choices).

Or programmatically, it's being thrown from utils:::.getHelpFile(help("visualize")).

This helps a ton. I'm sure we can find a way around this. I'll follow up once I've got something working.

@vnijs
Copy link
Collaborator

vnijs commented Sep 5, 2019

I see. Well spotted @dgkf. Could the lookup be restricted to packages that are in search()?

@dgkf
Copy link
Contributor Author

dgkf commented Sep 5, 2019

We can figure out which package the function is being pulled from (the same way we do when we do the function name completion and generate the package name on the right of the dropdown). With this info we can prevent the ambiguity in the help search. I had just forgotten to collect and pass this extra data back to the tooltip handler. It's now been added and the way the help files are searched has been cleaned up.

I think we might still see the message if there are redundant help names in base. I don't think that's the case, but I'm not intimately familiar with everything in base. The number of situations when someone may see it in the debug output should be much reduced.

@vnijs
Copy link
Collaborator

vnijs commented Sep 5, 2019

Nice! @detule Could you try out the dev branch @ https://github.com/dgkf/shinyAce to see if there are any issues with modules? If that seems to work as expected I'd like to merge this PR into master.

Again, great work @dgkf!

@vnijs
Copy link
Collaborator

vnijs commented Sep 5, 2019

@dgkf It seems the same error we saw with visualize can also occur with datasets (e.g., diamonds). FYI I include a smaller version of the full diamonds data that is part of ggplot

image

@dgkf
Copy link
Contributor Author

dgkf commented Sep 5, 2019

I see. It turns out this because there is both a ggplot2::diamonds and a radiant.data::diamonds. Unlike functions from these namespaces, the attached datasets don't carry with them an environment, so I haven't found a way to differentiate the two.

Instead, the function I use to pull up the help content now will take only the first available help file (the most recently attached). I think this is the preferred behavior.

To try to keep some of these various testing scenarios embedded with the software, I also added a handful of unit tests to check for some of these edge cases.

@vnijs vnijs merged commit 5aa49a1 into trestletech:master Sep 5, 2019
@vnijs
Copy link
Collaborator

vnijs commented Sep 5, 2019

@dgkf I just merged the PR. Thanks again for these great additions to shinyAce! @saurfang and @detule if would care to test if you see any issues with your apps that use shinyAce that would be great. I'm aiming to push the updated version to CRAN sometime next week.

@vnijs
Copy link
Collaborator

vnijs commented Sep 28, 2019

FYI shinyAce 0.4.1 is being built on CRAN now. Thanks again @dgkf for your contributions!

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

3 participants