Brush fixes and enhancements

[dvg-p4]

TODO

• Merge in changes from the last two shiny releases
• Resolve any non-trivial conflicts
• Merge in more changes from the latest commits to main
• Resolve those conflicts
• Test

Breaking changes

• Closed Session$resetBrush Does Not Work For Shiny Modules #2866: session$resetBrush() now properly namespaces the brush ID automatically within a module. This breaks previous workarounds, which will now result in a doubly-namespaced brush ID. This function has also been supplemented with the slightly more user-friendly shiny::resetBrush(); see details below.

New features and improvements

• Added resetBrush() function for a slightly more user-friendly alternative to session$resetBrush() with syntax similar to the update*() functions.

• Closed Set ggplot2's plot brush programmatically Shiny #1456: Added updateBrushCoords() function to programatically update the area brushed on a plot, similar to other update*() functions. Tested primarily with ggplot2, but may work with reduced functionality with base graphics and images. See help(updateBrushCoords, shiny) for more details.

Bug fixes

• Closed Double redraws for brush #2344 and Moving/resizing brush is not debounced if plot redraws #1642: Overhauled brush code so that brush handlers (as well as click/hover handlers) persist when a plot is redrawn, and consistently move any drawn brushes to the correct location if the plot is resized. This fixes an issue where a plot that's redrawn in response to a brush would circumvent the debouncer and sometimes update twice each time the brush was moved--or worse, constantly re-update itself as the brush was being dragged. This also fixes a frustrating-to-replicate bug where the rectangle drawn on the plot could become duplicated and desynced from the coordinates sent to the server with a series of rapid user inputs.

[dvg-p4]

I've merged in the latest main, this appears to be working again. The one failure is not the fault of this PR, the latest main commit also fails the 3.5.3 test

[dvg-p4]

joshuaulrich/xts@1ea13d4 seems to be the source of the 3.5.3 failure--looks like a dependency of a dependency added in an R >= 3.6.0 requirement a few weeks ago. Annoying that the dependency resolution won't take a version of it prior to the explicit R dependency being added.

[dvg-p4]

@wch is there anything else this would need to get it merged in? I'm aware that this is a pretty huge PR, but it does solve a number of longstanding issues with the "brush" functionality.

[dvg-p4]

Removing "merge in the fix for R3.5" from the TODO since that doesn't actually seem likely to be fixable given the way that CRAN handles (or really, fails to handle) older versions of packages. Unless someone convinces the xts folks that throwing a fit over the fact that dplyr is installed harms the user experience more than it helps it, and they take out that R >= 3.6.0 in their next release. Or if shiny removes the dygraphs suggestion, though that's subideal given there seems to have been some significant effort towards integration.

[dvg-p4]

Does look like there are more commits to main for me to merge in though

[CLAassistant]

All committers have signed the CLA.

[dvg-p4]

@mjakubczak looks like they updated the CLA and want everyone to re-sign it. Since I based this PR off of yours from three years ago, it looks like they want your signature as well as mine--would you mind doing so?

[mjakubczak]

@dvg-p4 done ;)

[dvg-p4]

Thanks! alright, now i just gotta...merge the last two releases of shiny into here, resolve whatever merge conflicts result, and see if I can get the core maintainers to notice me lol

[gadenbuie]

Hi @dvg-p4, above all else I want to acknowledge the amount of work you've put into this PR; it's quite impressive. We appreciate the effort you put in, both in the PR itself, and in keeping it up to date over a few releases where this PR was not included or reviewed.

I think the biggest challenge this PR faces is that it's too large and covers too much ground. Davis Vaughn wrote an excellent chapter on the advantages of small, focused PRs and many of the concepts of that chapter apply here.

In particular, in Shiny, reviewers have a primary goal of making sure that changes are correct and scoped. Reviewing is challenging because this region of the code is complicated and involves many features, and anyone reviewing changes in this area will have to spend considerable time reading and gaining context on how this all works. And in addition, we have to think through the many ways and places where Shiny components can be used as well as browser compatibility, etc. When PRs are smaller and scoped, a review can be much more confident that they've understood the change, and it's a lot easier to assess the amount of work it will take to review in advance, which increases the chances of a timely review.

I understand that breaking this PR into smaller changes is a lot of work and I would equally understand if you're not up for it at this point, especially after such a long delay in hearing meaningful feedback. I also understand, from personal experience, how changes in Shiny tend to layer (once change opening the door to another), which makes it even more difficult to untangle a large PR into smaller PRs. In my experience, though, it's always worth the effort when it makes the PR more approachable and easier to review. That said, from reading the PR description, it does seem likely that some parts of this PR could naturally be pulled out into smaller, individual PRs.

[dvg-p4]

Hi @gadenbuie,
Thanks for the feedback! I'd mostly assumed that was the case--this is admittedly a monster of a PR. Many of the changes here are unfortunately interdependent, but there's a few incidental fixes and additions (updateBrush(), the namespacing thing) that I could probably break out into a separate PR. It's been a while since I've taken a look at this, so yeah it'll take some time to re-understand my own changes :-), but hopefully I'll be able to get around to it at some point.