ArgoCanada / argoFloats

Tools for analyzing collections of oceanographic Argo floats
https://argocanada.github.io/argoFloats/index.html
17 stars 7 forks source link

get someone to try `mapApp()` to see if it "feels" good #486

Closed dankelley closed 3 years ago

j-harbin commented 3 years ago

@cgrdn & @paleolimbot if you have time, would you be interested in doing this? mapApp() is a shiny app within argoFloats and that's all I will say. We are curious from the help docs, etc. if the use of mapApp() is self-explanatory, does what you expect it should do, and what you want it to do?

We are tentatively planning for a release to CRAN sometime next week, so it'd be great if this could be done by Wednesday, again only if you have time :)

j-harbin commented 3 years ago

To that last point, it should be noted that the "develop" branch is the branch to use for the most up to date version of mapApp

paleolimbot commented 3 years ago

I just did a fresh install on a fresh computer and it worked well! The only thing that was odd was that the checkboxes "dissapear" when checked and I don't know why!

Screen Shot 2021-08-20 at 3 03 31 PM Screen Shot 2021-08-20 at 3 03 37 PM
dankelley commented 3 years ago

I see that disappearance also, but only in RStudio ... if I click the button to view in a browser, it seems okay. It seems to happen in any app I make.

cgrdn commented 3 years ago

Hi just wanted to comment that I haven't forgotten about this and I will have a look at it this afternoon!

cgrdn commented 3 years ago

I was actually in good position to do this because my computer recently had to be wiped clean recently so this went from a fresh install:

  1. I installed argoFloats no problem, but then ran into an error Error in mapApp() : must install.packages("colourpicker") for this to work. Not sure if that is just a fact of life or if that means that package should be listed in the dependencies somewhere (since everything else I needed was installed when I ran install_github("ArgoCanada/argoFloats", ref="develop"). DONE
  2. After installing that, I ran into a similar but slightly different issue where I ran it, and it got as far as creating a window but then I got the following error: Error in find.package(package, lib.loc, verbose = verbose) : there is no package called ‘ocedata’. Command stalled there and I had to use the stop sign to escape. Again not sure if this is just the kind of thing you run into installing a new package or if that should be a dependency. DONE
  3. After resolving that it ran a little longer but ran into an issue while downloading the global index: DONE
Warning: Error in argoFloats::getIndex: must install.packages("lubridate") for getIndex() to work
  50: stop
  49: argoFloats::getIndex
  48: server [C:/Users/GordonC/Documents/R/win-library/4.1/argoFloats/shiny/mapApp/app.R#172]
Error in argoFloats::getIndex(age = age, destdir = destdir, server = argoServer,  : 
  must install.packages("lubridate") for getIndex() to work
  1. Once I installed that last thing it was great and I thought very intuitive to use. I put in a float ID and it helpfully suggested I change to single view. I did run into an issue where I did something or unselected something and it seemed to continually refresh, to the point where I had to close it. I haven't been able to recreate this problem though. We'll ignore this for now, because we cannot reproduce it. Of course, this could be an issue.
  2. I would have liked a button to totally reset the map after I was done looking at my float. This is done by the "r" keystroke. Based on this comment we realized we needed to display keystroke better. This was done in https://github.com/ArgoCanada/argoFloats/issues/489
  3. My last nit picky points that I don't think actually need to be addressed but I will pass on in case its helpful:
    • 6.1 I wanted my mouseclick to preserve information somewhere when looking at a single float. Even though its stated for me that mouse hover does it, I still kind of expected clicking to do something We addressed this in issue https://github.com/ArgoCanada/argoFloats/issues/491, where we now make the "holding" of the information stand out more
    • 6.2 I went looking for the zoom buttons more "on" the map at first. I'm sure that's a bigger ask for than it sounds like it is. Zooming is done by key strokes, but this comment made us realize we needed to make key strokes stand out more. Again, this was addressed in https://github.com/ArgoCanada/argoFloats/issues/489
dankelley commented 3 years ago

Thanks very much, Chris. Your comment has two parts: packages and functions. Since github doesn't have threaded comments, I propose to focus on the first to begin with, returning to the second after we get progress on the first.

NOTE: I'd love to hear from @paleolimbot on what I'm writing in this comment. He knows a great deal about such things.

The need to install things was by design, because the package as a whole does not need them -- only mapApp() needs them. Well, that was the initial reasoning, anyway, and we could of course change it.

The downside of requiring other packages (done by naming them in the "Depends" field of the DESCRIPTION file) is that then when the user executes calls library(argoFloats), the other packages will be loaded. That seems benign, but packages often have functions of identical names, and so the user will see as below. I think that might actually be more confusing than the message we have at the moment. From my own experience, the first time I saw a masking message like this I was confused.

What I do not know, and Dewey may confirm that I have listened to explanations many times (!) is whether we can get other packages installed automatically without having them loaded. (Maybe putting them into Imports is the right way? Sorry, Dewey.)

> library(argoFloats)
Loading required package: colourpicker
Loading required package: curl
Using libcurl 7.77.0 with LibreSSL/2.8.3
Loading required package: lubridate

Attaching package: ‘lubridate’

The following objects are masked from ‘package:base’:

    date, intersect, setdiff, union

Loading required package: ocedata
Loading required package: testthat
Loading required package: ncdf4
Loading required package: shiny

Attaching package: ‘shiny’

The following object is masked from ‘package:colourpicker’:

    runExample
paleolimbot commented 3 years ago

I think you've got the right packages in the right places... perhaps checking for all of them at the start of Map app and giving a single error reporting the missing combination of the way to go?

dankelley commented 3 years ago

I agree, and have coded for that, but not yet pushed.

j-harbin commented 3 years ago

Thanks Chris and Dewey! This has been very insightful. Dan, I'm not working on argoFloats "stuff" today so your push won't conflict with anything I'm doing.

paleolimbot commented 3 years ago

I imagine it's outside the scope of a CRAN release this week, but you could use leaflet::renderLeafelet() / leaflet::leafletOutput() to get the pan/zoom/click behaviour Chris mentioned

dankelley commented 3 years ago

Thanks for that Dewey. I think I want it for something else, anyway!

PS. I think CRAN is sort of on holiday this week (from a note on https://stat.ethz.ch/mailman/listinfo/r-package-devel I saw today)

dankelley commented 3 years ago

Now, mapApp() checks and reports on all packages that are used anywhere in argoFloats. That way, a newcomer who tries using mapApp() will get instructions for all packages at once, avoiding the start/stop problem that Chris encountered.

I've added this, starting at https://github.com/ArgoCanada/argoFloats/blob/aeb8cc965775e05608c7b5670716285863bc4e01/R/mapApp.R#L71:L76

dankelley commented 3 years ago

@cgrdn I added numbers to your comment, for cross-referencing. I think I've addressed numbers 1, 2 and 3. Perhaps when @j-harbin has an argo day, we can discuss the others. (I'm not asking for a N-person meeting because they are so hard to set up ... and they will get a lot harder pretty soon, when teaching starts!)

j-harbin commented 3 years ago

@paleolimbot and @cgrdn, thank you both very much for taking the time to look into this. Based on your comments, we have made some tweaks which were addressed in bold font inside of Chris's comment. Dan, since you were the one who created this issue, if you are satisfied, feel free to close it. Of course if anyone has any other comments/concerns feel free to mention.

dankelley commented 3 years ago

I agree that the issue, as described by its title, has been addressed, and therefore I am closing it.

PS. this "described by its title" part is important. I find that's the best way to avoid issues turning into months-long dialogues that are difficult (for me) to keep track of.