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

Fix "Viewer.buttons is deprecated" warning issue #2201

Merged
merged 2 commits into from Sep 28, 2022

Conversation

joedf
Copy link
Contributor

@joedf joedf commented Sep 11, 2022

Removes the erroneous Viewer.buttons is deprecated; Please use Viewer.buttonGroup warning. See explanation in #2193 ...

@joedf joedf changed the title based on @pearcetm 's fix Fix "Viewer.buttons is deprecated" warning issue Sep 11, 2022
Copy link
Member

@iangilman iangilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for taking this on!

I don't think this is enough. What we really want is to be copying the getters and setters into the new object, but the current state of this patch will copy undefined for anything that has a getter (since the property descriptor for something with a getter has no value). That does indeed fix the warning, but it'll lead to unintended behaviors.

I've messed around, and I think this works:

                    var descriptor = Object.getOwnPropertyDescriptor(options, name);
                    if (descriptor.get || descriptor.set) {
                        Object.defineProperty(target, name, descriptor);
                        continue;
                    }

                    copy = descriptor.value;

                    // Prevent never-ending loop
                    if ( target === copy ) {
                        continue;
                    }

                    // Recurse if we're merging plain objects or arrays
                    if ( deep && copy && ( OpenSeadragon.isPlainObject( copy ) || ( copyIsArray = OpenSeadragon.isArray( copy ) ) ) ) {
                        src = target[ name ];

Here's what it looks like as a diff in the context of the code:

image

What I found is that both target[name] and options[name] can trigger the warning. To take care of the options version, we are getting the descriptor and then setting the property on the target if needed. To take care of the target version, I'm simply moving it into the only clause where it's being used, and from there it doesn't seem to trigger any of our getters.

@joedf What do you think?

@joedf
Copy link
Contributor Author

joedf commented Sep 17, 2022

@iangilman I've put your suggested changes in. Seems to work great!

Copy link
Member

@iangilman iangilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome... Thank you for verifying the fix and getting it in!

@iangilman iangilman merged commit 9ec9189 into openseadragon:master Sep 28, 2022
iangilman added a commit that referenced this pull request Sep 28, 2022
joedf added a commit to joedf/openseadragon that referenced this pull request Sep 29, 2022
commit 0474d64
Author: Ian Gilman <ian@iangilman.com>
Date:   Wed Sep 28 14:00:46 2022 -0700

    Changelog for openseadragon#2201

commit 38bc89f
Merge: f88a1f0 9ec9189
Author: Ian Gilman <ian@iangilman.com>
Date:   Wed Sep 28 13:57:06 2022 -0700

    Merge branch 'master' of github.com:openseadragon/openseadragon

commit 9ec9189
Merge: d3ef767 6d74b68
Author: Ian Gilman <ian@iangilman.com>
Date:   Wed Sep 28 13:54:41 2022 -0700

    Merge pull request openseadragon#2201 from joedf/fix-2193

    Fix "Viewer.buttons is deprecated" warning issue

commit 6d74b68
Author: Joe DF <3848219+joedf@users.noreply.github.com>
Date:   Sat Sep 17 17:10:03 2022 +0200

    implement @iangilman 's fix

commit 7497b83
Author: Joe DF <3848219+joedf@users.noreply.github.com>
Date:   Sat Sep 10 21:31:15 2022 -0400

    based on @pearcetm 's fix

commit f88a1f0
Author: Ian Gilman <ian@iangilman.com>
Date:   Tue Aug 23 14:17:39 2022 -0700

    Changelog for openseadragon#2136
@joedf joedf deleted the fix-2193 branch September 29, 2022 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants