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

Failures when calling ggedit::ggedit() without attaching the package #17

Closed
krlmlr opened this issue Jun 1, 2017 · 19 comments
Closed

Comments

@krlmlr
Copy link

krlmlr commented Jun 1, 2017

library(ggplot2)
p <- ggplot(mpg) + geom_bar(aes(x = class))
ggedit::ggedit(p)


Warning: Error in if: argument is of length zero
Stack trace (innermost first):
    102: shiny::renderPlot [/tmp/Rtmp2s9CU6/R.INSTALL1b12402fb480/ggedit/R/ggeditGadget.R#219]
     92: <reactive:plotObj>
     81: plotObj
     80: origRenderFunc
     79: output$Plot
      4: shiny::runApp
      3: shiny::runGadget
      2: ggeditGadget [/tmp/Rtmp2s9CU6/R.INSTALL1b12402fb480/ggedit/R/ggeditGadget.R#338]
      1: ggedit::ggedit [/tmp/Rtmp2s9CU6/R.INSTALL1b12402fb480/ggedit/R/ggedit.R#125]

@yonicd
Copy link
Owner

yonicd commented Jun 1, 2017

it has to have an object to run on ggedit::ggedit(ggedit::pList[1])

@krlmlr
Copy link
Author

krlmlr commented Jun 1, 2017

Does the documentation need to be adapted then? (I'm using the CRAN version, which is fairly recent, but I still may be missing a change.)

@yonicd
Copy link
Owner

yonicd commented Jun 1, 2017

its under heavy revision the last month (generalizing to support ggextensions).

but the above should be the solution in the cran version

@yonicd
Copy link
Owner

yonicd commented Jun 1, 2017

this is the current documentation

https://metrumresearchgroup.github.io/ggedit/console.html

@krlmlr
Copy link
Author

krlmlr commented Jun 1, 2017

To recap: How do I need to change my original code:

library(ggplot2)
p <- ggplot(mpg) + geom_bar(aes(x = class))
ggedit::ggedit(p)

@yonicd
Copy link
Owner

yonicd commented Jun 1, 2017

Depends:
    R (>= 2.3.0),
    ggplot2 (>= 2.2.0),
    shinyBS,
    shiny,
library(ggplot2)
library(shinyBS)
p <- ggplot(mpg) + geom_bar(aes(x = class))
ggedit::ggedit(p)

@krlmlr
Copy link
Author

krlmlr commented Jun 1, 2017

I don't think I should be attaching shinyBS just to run your package. Is there a way to avoid this?

@yonicd
Copy link
Owner

yonicd commented Jun 1, 2017

i tried moving it now from depends to imports and using importsFrom and it still is being persistent on needing shinyBS to be attached.

I'll look into it a bit more later today.

thank you for raising the issue.

@krlmlr
Copy link
Author

krlmlr commented Jun 1, 2017

Thanks for your very prompt response!

@yonicd
Copy link
Owner

yonicd commented Jun 1, 2017

could it be that shinyBS needs to load bootstrap.js and must be attached to do so?

https://github.com/ebailey78/shinyBS/tree/shinyBS3/inst/www

@krlmlr
Copy link
Author

krlmlr commented Jun 1, 2017

But then shinyBS should be able to do so without being attached. If you could pull together a reproducible example that shows how shinyBS fails if it's not attached, best without involving ggedit, this would help shinyBS maintainers to fix the problem.

I assume you're importing shinyBS via requireNamespace() or a namespace import?

@yonicd
Copy link
Owner

yonicd commented Jun 1, 2017

using roxygen2 importFrom shinyBS bsModal

@krlmlr
Copy link
Author

krlmlr commented Jun 1, 2017

Yes, that should be sufficient to properly load it on time.

@ebailey78: Any idea why we are required to attach shinyBS to run ggedit?

@yonicd
Copy link
Owner

yonicd commented Jun 2, 2017

i added a patch to get around shinyBS not loading. requireNamespace('shinyBS') in the ggedit call, so it works now in the dev version. But I would still like to hear ebailey's solution

@yonicd
Copy link
Owner

yonicd commented Jun 2, 2017

@jcheng5 maybe you know the answer to this question

@jcheng5
Copy link
Contributor

jcheng5 commented Jun 2, 2017

Sorry, I don't. I don't know anything about shinyBS.

@jcheng5
Copy link
Contributor

jcheng5 commented Jun 2, 2017

Oh, I do know. This is why: https://github.com/ebailey78/shinyBS/blob/c329f8ce43e44579cafbb16fc3109fb69d403e57/R/misc.R#L1-L6

One easy fix would be to change that from onAttach to onLoad. Possibly a better fix would be to stop using addResourcePath at all, and have the dependency objects point to their locations on disk. I wouldn't rule out there being a good reason why @ebailey78 did it this way, but it doesn't immediately occur to me what it is (less copying?).

@yonicd
Copy link
Owner

yonicd commented Jun 2, 2017

btw shinyAce also is showing the same problem if you dont manually attach shinyBS prior to running a shinyapp

@yonicd
Copy link
Owner

yonicd commented Oct 2, 2017

got around the problem in shinyBS by adding onLoad hooks

closing

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

No branches or pull requests

3 participants