mapedit icon indicating copy to clipboard operation
mapedit copied to clipboard

Closing the browser window after a selection is done in selectFeatures

Open lbusett opened this issue 7 years ago • 26 comments

Hi,

I am calling selectFeatures from the button of a GUI-based application, using viewer = browserViewer(browser = getOption("browser")) to allow selection from a browser.

All works well, and when I click "DONE" the selected features are passed back to the caller. The only problem is that after I click DONE the browser window is greyed out but not closed, so that it appears that the "application" crashed.

You can replicate the problem using:

nc <- st_read(system.file("shape/nc.shp",package="sf"))
mapedit::selectFeatures(nc, viewer = browserViewer(browser = getOption("browser"))

Is there any way to automatically close the window (i.e., the browser tab) when the selection is completed?

Thanks in advance!

PS: I am working with the current CRAN version of mapedit (0.3.2) becausethis functionality should be implemented in a CRAN package, so using the github version is not a feasible option

lbusett avatar Feb 24 '18 08:02 lbusett

Yes. I think that is possible. I will investigate when I am back at a computer. Likely next week

tim-salabim avatar Feb 24 '18 11:02 tim-salabim

@lbusett I agree this would be much more pleasant behavior. I think I am near a solution, so hopefully can post a proof of concept by end of day.

timelyportfolio avatar Feb 24 '18 13:02 timelyportfolio

@lbusett, https://github.com/timelyportfolio/mapedit/commit/8c2a4ba5b76f574ff3e9961ee5484a01d9ba2d94 should add the suggested behavior. Before we submit to CRAN, I would like to test further and allow some time for feedback. Although you say

" I am working with the current CRAN version of mapedit (0.3.2) becausethis functionality should be implemented in a CRAN package, so using the github version is not a feasible option "

is there any way you might help us test using devtools::install_github("timelyportfolio/mapedit@autoclose")?

If not, hopefully some other users will help us test.

timelyportfolio avatar Feb 24 '18 13:02 timelyportfolio

@timelyportfolio I just tested, and it seems to work perfectly! Thank you very much!

Concerning the CRAN issue, I was mentioning it just because I would like to include the functionality on a release I am planning for next week, so I was wondering if there was a way to do this using the current CRAN mapedit version. However, that's not really a problem. I will document the current behavior using a message to the user, explaining that it will change on next mapedit version, or if they install the github one.

Thanks for the very quick fix!

lbusett avatar Feb 24 '18 14:02 lbusett

I am not opposed to doing a CRAN release soon before the next major release with leaflet > 1.0. @tim-salabim, how about I assemble a list of changes for this minor release to see where we stand?

timelyportfolio avatar Feb 24 '18 15:02 timelyportfolio

I totally agree. We have been dormant for a long time. I also have to submit the quarterly progress report soon, so a release would be great.

tim-salabim avatar Feb 24 '18 16:02 tim-salabim

@timelyportfolio on my linux machine I cannot reproduce the intended behaviour (browser tab only greyed out, not closed). Any tips for debugging?

tim-salabim avatar Feb 27 '18 17:02 tim-salabim

Just to mention that my test was done on Windows 10 + chrome. I could also test on Linux tomorrow at work.

lbusett avatar Feb 27 '18 19:02 lbusett

Mine was with an already open firefox (hence new tab) on ubuntu 16.04

tim-salabim avatar Feb 27 '18 20:02 tim-salabim

Ok, I can confirm that the close.window() call works in chrome (on my ubuntu). Firefox, however, seems to not allow this to happen unless the window was opened by a JS script. See the Mozilla developper page. The solution proposed here (which I've seen elsewhere too) doesn't seem to work, as the comment suggests.

Any ideas @timelyportfolio ?

I think, if there is no workaround, at the very least we need to make it very clear in the help file that this will not work in firefox.

@lbusett Do you have a chance to test with firefox on windows?

tim-salabim avatar Feb 28 '18 11:02 tim-salabim

I've found a way to get it to work in firefox:

1.input "about:config " to your firefox address bar and enter; 2.make sure your "dom.allow_scripts_to_close_windows" is true

found here - just below the first code snippet.

@timelyportfolio if this is the only solution, then I think it is fine to keep things like they are but clearly outline in the help page how this can be achieved. What do you think?

tim-salabim avatar Feb 28 '18 11:02 tim-salabim

@tim-salabim thanks so much for working through this. I wonder where we would add this help item. I guess we could discuss on the blog post.

timelyportfolio avatar Feb 28 '18 12:02 timelyportfolio

@timelyportfolio I've added a comment everywhere we have argument viewer. Can you double-check if this is understandable and good to go (to CRAN)?

tim-salabim avatar Feb 28 '18 12:02 tim-salabim

Hi.

I confirm all works well on Linux/Chrome. On Windows/Firefox, I get the "greyed-out/no close" behavior too. Could not yet try the workaround.

lbusett avatar Feb 28 '18 14:02 lbusett

The instructions seem cleat to me, though. Will try this evening.

lbusett avatar Feb 28 '18 14:02 lbusett

Just to confirm that the Firefox workaround works also on Windows.

lbusett avatar Feb 28 '18 21:02 lbusett

Sweet! Thanks @lbusett ! I will submit to CRAN tomorrow then.

tim-salabim avatar Feb 28 '18 21:02 tim-salabim

Hi,

just a quick note: In case the browser is closed while mapedit is waiting for "Done" or "cancel" to be selected, "R" gets stuck in a limbo where it is waiting for a response that can no longer be given ("Listening on http://127.0.0.1:4039").

The only way I found to "resuscitate" the session if I accidentally close the browser window is "focusing" RStudio and pressing "ESC".

I do not know if this can have a solution (and I guess it would also affect mapedit windows opened in RStudio viewer if the user closes the viewer using the "x" red button instead of pressing "cancel"), but I thought it was worth mentioning.

HTH.

lbusett avatar Mar 06 '18 11:03 lbusett

@lbusett, interesting, and I should have thought about that. I will work on some potential solutions.

timelyportfolio avatar Mar 07 '18 01:03 timelyportfolio

@lbusett, will you test 106a643 by devtools::install_github("timelyportfolio/mapedit")? I added comments inline, but feel like I should communicate here as well. If a user has two sessions open then closing either will kill the other. For instance, if I editMap in RStudio Viewer and then click open in browser and then close browser the RStudio viewer session will also end. Any work done there would be lost. I think this is fine and the benefits of this change outweigh this very small cost, but I wanted to mention and get feedback.

timelyportfolio avatar Mar 07 '18 12:03 timelyportfolio

@timelyportfolio Sure. I'll have a look ASAP.

lbusett avatar Mar 07 '18 13:03 lbusett

@timelyportfolio looks like it's working (windows 10, chrome), but with a caveat: when I force-close the browser tab in selectFeatures, the current selection is returned as if I clicked "Done". In my opinion, it would make more sense if the behavior of "closing the tab" would mimic that of clicking "Cancel", instead. What do you think?

lbusett avatar Mar 07 '18 20:03 lbusett

@lbusett thanks so much for checking. I am agnostic; @tim-salabim, do you have an opinion? Funny, I actually put in a comment questioning what is best.

timelyportfolio avatar Mar 07 '18 21:03 timelyportfolio

@lbusett is there any disadvantage of returning what has been selected/drawn so far? I can only think of the advantage that an accidental close does not lose what has been done.

tim-salabim avatar Mar 08 '18 06:03 tim-salabim

@tim-salabim I'd guess it is a matter of "user expectations". Usually, when you edit something inside a program and the program closes "unexpectedly", the changes are not saved. Also, if you close a program using the "x" buttons, edits are not automatically saved, but you are at least asked if you want to keep the edits or discarded.

In my opinion, having the changes saved if you close the editing window is therefore not a good idea. Imagine for example that you have an "application" in which you edit a vector file and "save" the updates if you click "Done". With the current functionality, the updates would be saved even if you close the browser, or the browser somehow crashes (that is, without an explicit confirmation by the user), which I think would confuse the users (well, at least it would confuse me....).

I think therefore that sending a NULL response in case the browser is closed would be better. Otherwise, I think the user should at least receive a "request" asking if he wants to confirm or discard the edits.

Just my 2 cents !

lbusett avatar Mar 08 '18 08:03 lbusett

Good points! @timelyportfolio we're diving more and more into GUI development here... Is it possible to use e.g. prompt (I have already played with YDKJS ;-) ) to get user confirmation?

tim-salabim avatar Mar 08 '18 08:03 tim-salabim