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 dismissalert to remove arbitrary number of messages #30

Closed
wants to merge 5 commits into from

Conversation

dipterix
Copy link

@dipterix dipterix commented Jul 4, 2020

This PR is related to issue #29

A simple demo showing how it works:

library(shiny)

ui <- fluidPage(
  shinyalert::useShinyalert(),
  actionButton('a','Ok')
)

server <- function(input, output, session) {
  observeEvent(input$a, {
    shinyalert::shinyalert(
      'Warning',
      'Click OK to run code',
      callbackR = function(...){
        # procedure 1 runs for 3~4 seconds
        Sys.sleep(3 + runif(1))
        # Now dismiss modal
        shinyalert::dismissalert()
      }
    )
    shinyalert::shinyalert(
      'Scheduled!',
      'R is running, please wait for the result...', 
      showConfirmButton = FALSE
    )
  })
}

shinyApp(ui, server)

@dipterix dipterix marked this pull request as ready for review July 4, 2020 07:59
@dipterix
Copy link
Author

dipterix commented Jul 8, 2020

Changes:

  1. Renamed dismissalert to closeAlert
  2. Added immediate option to shinyalert
  3. closeAlert can choose number of alerts to be closed, or a specific alert with cbid given
  4. Fixed some pre-existing doc issues.

I added myself to DESCRIPTION as ctb (contributor, providing piece of code) as convention, you can remove it if it causes some legal issues.

@dipterix dipterix mentioned this pull request Jul 8, 2020
@@ -253,10 +265,34 @@ shinyalert <- function(
}
}, once = TRUE)
params[['callbackR']] <- NULL
} else {
# Still create `cbid` used by closeAlert to explicitly close this alert
Copy link
Owner

Choose a reason for hiding this comment

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

In the if statement I added a comment that the serialization isn't happening to prevent a performance issue #19

Did you notice that and test if this change is going to re-introduce that issue?

Copy link
Author

Choose a reason for hiding this comment

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

I don't recommend params <- as.list(environment()) at the very beginning. Chances are too small for two alerts sharing the same cbid with runif postfix. If you get rid of the enclosing environment, digest::digest will be quite fast as everything else should be basic types and contain no environment. Alternatively you could use utils::capture.output({print(environment())}) to distinguish environments because the runtime environment of R functions will unlikely to be named, meaning its memory address will be printed, resulting in something like "<environment: 0x7f802d60a1b0>". Serializing string is much faster than serializing an environment.

Copy link
Author

Choose a reason for hiding this comment

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

Or, if you do want to keep params <- as.list(environment()), you can create random cbid with no digests for alerts without callbacks. The returned cbid can be used to close that specific alert or writers can use that cbid to generate their own callbacks outside of shinyalert.

Copy link
Owner

Choose a reason for hiding this comment

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

The line params <- as.list(environment()) is not for serialization or creating a unique ID, it was simply a way to get a list of all the function arguments values - default values for arguments that weren't provided and the user-supplied values for the arguments that were provided. Do you know of an alternative way to get all the arguments of the current function as a list?

But anyway, you're right that the digest isn't actually required , it was only to add some randomness. Instead of using the params hash plus a random integer, it might be better to just use the uuid package

Copy link
Author

Choose a reason for hiding this comment

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

In that way you are right, but your function arguments contains no function except callbackR, which will be removed before serialization. Therefore serialization should be fast. Also personally I think using runif is good enough to create unique IDs unless people set seed before calling the function. In that case, you might want to consider utils::capture.output({print(environment())}) I proposed to ensure different IDs created.

Copy link
Owner

@daattali daattali left a comment

Choose a reason for hiding this comment

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

Thanks for the great PR!

I apologize for being so picky but I do have a few things I would ask of you before merging:

  • Remove all changes that are not directly related to closeAlert
  • Files should end with a newline
  • The immediate parameter is related but not should not go in the same PR as closeAlert
  • It looks like some of the code (to create the callback id) can be reused instead of being used twice identically
  • I don't see the use for the n argument in closeAlert. It seems very esoteric - what's the use case for that? I would prefer to just have cbid argument and if none is provided then you close everything

// Always create cbid
var cbid = params['cbid'];
delete params['cbid'];
if(typeof cbid === 'string'){
Copy link
Owner

Choose a reason for hiding this comment

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

Is cbid not alwasy going to be a string?

Copy link
Author

Choose a reason for hiding this comment

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

We can remove the if statement if you always include cbid in R code.

Copy link
Owner

Choose a reason for hiding this comment

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

It wasn't before, but my understanding is that your PR ensures that cbid is ALWAYS going to be in the javascript parameters, in which case this check is redundant. Is that right?

callbackR = function(value) {
Shiny.onInputChange(cbid, value);
}
}

var callback = function(value) {
// Remove instance immediately
var ii = 0;
Copy link
Owner

Choose a reason for hiding this comment

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

Please use a better variable name

@daattali
Copy link
Owner

I made a commit that uses uuid instead of hashing the params, so that's not an issue anymore. Thanks for bringing this to my attention

@daattali
Copy link
Owner

@dipterix let me know if you plan on continuing this or if I should take over!

@dipterix
Copy link
Author

dipterix commented Jul 13, 2020

I got fever. Please go ahead make changes if you feel necessary.

@daattali
Copy link
Owner

I'm working on some other packages this week, if you don't finish the PR by the end of the month I'll try to finish it :)

@daattali
Copy link
Owner

daattali commented Sep 7, 2020

@dipterix Are you able to finish this PR?

@daattali
Copy link
Owner

daattali commented Sep 8, 2020

I actually went through all your code and I'm mostly done integrating it. I might ask you for a PR soon :)

@dipterix
Copy link
Author

dipterix commented Sep 8, 2020

Hi @daattali , sorry for the late reply. The conflicts have been resolved.

@daattali
Copy link
Owner

daattali commented Sep 9, 2020

Merged in #37

@daattali daattali closed this Sep 9, 2020
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

2 participants