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

Brush fixes and enhancements #3683

Open
wants to merge 44 commits into
base: main
Choose a base branch
from

Conversation

dvg-p4
Copy link
Contributor

@dvg-p4 dvg-p4 commented Aug 13, 2022

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.

mjakubczak and others added 30 commits May 12, 2020 13:37
Discard all JS changes; take only R-side functions (mainly to maintain signatures)
double-renders once and for all
I just realized there's actually a system for the alpha version numbers, but I want to be able to easily distinguish my fork from the main branch for the time being.
Conflicts:
	NEWS.md
	inst/www/shared/shiny-autoreload.js
	inst/www/shared/shiny-showcase.css
	inst/www/shared/shiny-showcase.js
	inst/www/shared/shiny-testmode.js
	inst/www/shared/shiny.js
	inst/www/shared/shiny.js.map
	inst/www/shared/shiny.min.css
	inst/www/shared/shiny.min.js
	inst/www/shared/shiny.min.js.map
	srcts/src/imageutils/createBrush.ts
	srcts/src/imageutils/createHandlers.ts
	srcts/types/src/imageutils/createHandlers.d.ts
	srcts/types/src/utils/index.d.ts
Conflicts:
	inst/www/shared/shiny.js
	inst/www/shared/shiny.js.map
	inst/www/shared/shiny.min.js
	inst/www/shared/shiny.min.js.map
	srcts/src/imageutils/createBrush.ts
	srcts/types/src/imageutils/createBrush.d.ts
	yarn.lock
@dvg-p4
Copy link
Contributor Author

dvg-p4 commented Mar 15, 2023

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
Copy link
Contributor Author

dvg-p4 commented Mar 15, 2023

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
Copy link
Contributor Author

dvg-p4 commented Apr 18, 2023

@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
Copy link
Contributor Author

dvg-p4 commented May 25, 2023

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
Copy link
Contributor Author

dvg-p4 commented May 25, 2023

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

@CLAassistant
Copy link

CLAassistant commented Nov 8, 2023

CLA assistant check
All committers have signed the CLA.

@dvg-p4
Copy link
Contributor Author

dvg-p4 commented Nov 9, 2023

@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
Copy link

@dvg-p4 done ;)

@dvg-p4
Copy link
Contributor Author

dvg-p4 commented Nov 10, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants