regions plugin: fix region ignoring minLength when being resized #2002

sundayz · 2020-07-10T20:35:46Z

Short description of changes:

Fix region onResize code not checking minLength, which allowed you to "push" regions while dragging them past the point where they should stop shrinking. See related issue for better description.
Fix handles CSS in region example. They were set to width: 1% which made them scale with the size of the region, and they became invisible when the region was small enough. Now they're always 2px.

Breaking in the external API:

None

Breaking changes in the internal API:

None

Todos/Notes:

The resizing behaviour in general could do with some improvements. For example I don't think you should be able to drag regions to size 0, because then you can't grab them again and you lose them. Maybe regions could have a default minLength: 0.1 or so.

Other possible future enhancements:

minLength is not documented in the example page.
minLength/maxLength don't seem to be used a lot in the code, maybe they were never fully implemented?
Resizing delta is calculated by checking difference between current and last mouse position. It would be better if you could only resize the region when your mouse was near the handle (maybe calculate delta as handle position change)
Other functions which affect region size should probably check minLength, not just onResize

Related Issues and other PRs:

fixes #2001
closes #2008

…s small

coveralls · 2020-07-10T20:45:02Z

Coverage remained the same at 80.385% when pulling 653d8f6 on sundayz:master into b8cfabe on katspaugh:master.

thijstriemstra

thanks! can you also add a changelog entry?

example/regions/index.html

src/plugin/regions/region.js

osheroff · 2020-07-11T12:10:14Z

src/plugin/regions/region.js

            this.update({
                start: Math.min(this.start + delta, this.end),
                end: Math.max(this.start + delta, this.end)
            });
        } else {
+            // Check if changing the end by the given delta would result in the region being smaller than minLength
+            // Ignore cases where we are making the region wider rather than shrinking it
+            if (delta < 0 && Math.abs((this.end + delta) - this.start) < this.minLength) {


same question about Math.abs. Also curious why we have to check for delta > 0 and delta < 0; the checks are sorta fine, but was there a specific reason to put them in?

Since minLength isn't checked everywhere, it's actually possible to manually call this.update() and set the region to a size smaller than minLength. The delta check makes sure that if the area is too small, you can still make it bigger, but not shrink it further when dragging it with the mouse. A better fix would be to make sure the region can never be smaller than minLength, but I'm not familiar enough with this plugin to make that change.

…n width

marizuccara · 2020-07-11T13:23:08Z

Hi great idea and very useful! It would be even better if the minLength property was not related only to a single region which you define on initialization, but also when you create a region "manually", directly on the waveform. Because actually, for the regions you create with dragging, the problem persists. Maybe it would be better to define a general minLength prop, valid for all regions, even for the new ones that will be created in the future.

CHANGES.md

src/plugin/regions/region.js

marizuccara · 2020-07-18T10:59:12Z

Hi great idea and very useful! It would be even better if the minLength property was not related only to a single region which you define on initialization, but also when you create a region "manually", directly on the waveform. Because actually, for the regions you create with dragging, the problem persists. Maybe it would be better to define a general minLength prop, valid for all regions, even for the new ones that will be created in the future.

Hi, @sundayz, are you planning to open a PR for this, or can I proceed?

sundayz · 2020-07-19T12:21:10Z

@marizuccara You can do it if you'd like :) I will try to fix #2006 soon, should we do 1 PR to fix multiple issues with regions, or do a separate PR for each issue? #1925 also added something to regions in 4.0 which should be changed to a configuration option since it stops the seeker from working within regions

thijstriemstra · 2020-07-19T12:40:37Z

should we do 1 PR to fix multiple issues with regions, or do a separate PR for each issue?

One per issue, it makes it easier to track changes later on. I was thinking of doing a 4.1.0 release next week, but it can wait till regions plugin improvements you talk about are merged.

…spaugh#2002) * fix region not respecting minLength when being resized * fix region handle CSS in example being hard to grab when the region is small * Change region handle default width to 2px rather than 1% of the region width * Remove redundant math.abs check * add changelog entry (katspaugh#2002)

sundayz added 2 commits July 10, 2020 21:18

fix region not respecting minLength when being resized

3fbff4a

fix region handle CSS in example being hard to grab when the region i…

fc250d6

…s small

thijstriemstra requested review from osheroff and marizuccara July 11, 2020 11:53

thijstriemstra added the plugins label Jul 11, 2020

thijstriemstra changed the title ~~Fix region ignoring minLength when being resized~~ regions plugin: fix region ignoring minLength when being resized Jul 11, 2020

thijstriemstra suggested changes Jul 11, 2020

View reviewed changes

example/regions/index.html Outdated Show resolved Hide resolved

osheroff reviewed Jul 11, 2020

View reviewed changes

example/regions/index.html Outdated Show resolved Hide resolved

osheroff reviewed Jul 11, 2020

View reviewed changes

src/plugin/regions/region.js Outdated Show resolved Hide resolved

osheroff reviewed Jul 11, 2020

View reviewed changes

sundayz added 3 commits July 11, 2020 13:47

Change region handle default width to 2px rather than 1% of the regio…

64d3614

…n width

Remove redundant math.abs check

f5e7448

add changelog entry (#2002)

653d8f6

thijstriemstra reviewed Jul 11, 2020

View reviewed changes

CHANGES.md Show resolved Hide resolved

marizuccara reviewed Jul 11, 2020

View reviewed changes

src/plugin/regions/region.js Show resolved Hide resolved

thijstriemstra approved these changes Jul 12, 2020

View reviewed changes

marizuccara approved these changes Jul 13, 2020

View reviewed changes

thijstriemstra merged commit 7af166d into katspaugh:master Jul 13, 2020

marizuccara mentioned this pull request Jul 18, 2020

How to prevent a region handle from resizing beyond its opposite handle? #2008

Closed

marizuccara mentioned this pull request Jul 19, 2020

regions plugin: add regionsMinLength option #2009

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

regions plugin: fix region ignoring minLength when being resized #2002

regions plugin: fix region ignoring minLength when being resized #2002

sundayz commented Jul 10, 2020 •

edited

Loading

coveralls commented Jul 10, 2020 •

edited

Loading

thijstriemstra left a comment

osheroff Jul 11, 2020

sundayz Jul 11, 2020

marizuccara commented Jul 11, 2020

marizuccara commented Jul 18, 2020

sundayz commented Jul 19, 2020 •

edited

Loading

thijstriemstra commented Jul 19, 2020

regions plugin: fix region ignoring minLength when being resized #2002

regions plugin: fix region ignoring minLength when being resized #2002

Conversation

sundayz commented Jul 10, 2020 • edited Loading

Short description of changes:

Breaking in the external API:

Breaking changes in the internal API:

Todos/Notes:

Related Issues and other PRs:

coveralls commented Jul 10, 2020 • edited Loading

thijstriemstra left a comment

Choose a reason for hiding this comment

osheroff Jul 11, 2020

Choose a reason for hiding this comment

sundayz Jul 11, 2020

Choose a reason for hiding this comment

marizuccara commented Jul 11, 2020

marizuccara commented Jul 18, 2020

sundayz commented Jul 19, 2020 • edited Loading

thijstriemstra commented Jul 19, 2020

sundayz commented Jul 10, 2020 •

edited

Loading

coveralls commented Jul 10, 2020 •

edited

Loading

sundayz commented Jul 19, 2020 •

edited

Loading