search

ArgoCanada / argoFloats

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

Add undo button to mapApp() #521

Closed j-harbin closed 3 years ago

j-harbin commented 3 years ago

This will be going to the right of the Help button.

j-harbin commented 3 years ago

Done in commit 85c3db6e05260936e221cd3c318d72f8fda1f6c5 of develop.

dankelley commented 3 years ago

Q for @j-harbin do you find that, after you click Undo, the button stays gray? This happens for me, and it is slightly confusing.

Not quite related, but maybe Undo should only appear if sizeState() > 1L.

I have reopened this, so we don't have to search for the issue later. But, of course, feel free to reclose it, if you don't think these things matter much.

j-harbin commented 3 years ago

My button does stay grey, however, 1) I'm not sure how to fix that and 2) I think it's still clear what to do. I'm going to vote to leave this as it, but of course if you feel it's too confusing, by all means feel free to fix it.

In regards to only having the Undo button show up when sizeState() > 1L, I can see why this might be beneficial. I was thinking about what feels natural when doing undo, and when I think about what mvim does for example, when I click "u" a bunch of times and then I reach the point that nothing else can be done I get a message that says "Already at oldest change". Maybe that would be a better approach?

dankelley commented 3 years ago

As always, I agree with your "better approach". We can have it put up a warning in that case.

Also, at https://github.com/ArgoCanada/argoFloats/blob/c9d3cea2d783f78e5ac1f89cf7115ccbd729da7c/inst/shiny/mapApp/app.R#L815 I think we should only do the popState() and other things if sizeState() > 1L. There's no need (that I can think of) to re-assign into state, causing a redraw, if the results will be identical.

j-harbin commented 3 years ago

The warning message now pops up if sizeState() isn't greater than 1 and the user clicks the Undo button. The same can be said if the user presses "u". (commit 9ba8f433d7bf602bb5f0517967da8f00c7162dc7 of develop). I'm looking at your popState() comment.

j-harbin commented 3 years ago

I've made the suggested changes for popState() in commit e9dbdafb106718e4e1d19eb6377fa6e6de718db7 of develop. Instead of putting the condition for popState() and the undo notification based on a size of 1, it is instead based on 2. I'm not entirely sure why it needed to be two, but I've attached the debugging messages when popState() stops. When it was based on a size of 1, the undo message was showing up at the wrong time. Dan, you're more familiar with popState() etc, so although I consider this issue finish, I'll leave it open for you to look at. If you're satisfied feel free to close the issue. Screen Shot 2021-10-13 at 9 26 15 AM

dankelley commented 3 years ago

I'm guessing we need to start at 2 because the very first state is special, relating to that trick I made for the startup.

Can you change the code where it compares with 2 to instead compare with 2L? The reason is that this might be a bit clearer to someone looking at the code. (2L just means an integer 2, as distinct from 2.0 for example. Lots of core R functions specify the L to make things clearer and maybe faster.)

j-harbin commented 3 years ago

Fixed in commit 1dfb6aeeb468e91fa738cfcb57bf1611f2b22aa3 of develop.

j-harbin commented 3 years ago

Note the "Already at oldest change" message has been removed in commit a1851fa15faf3a14e05676307595f5f5d6d92d9d of develop. We believe the condition is based on if the app is in "begin" state or not. Sometimes the message was popping up correct, and sometimes it wasn't. We didn't want to confused the user and therefore decided to remove it.