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

New VSCode fallback behaviour for copyToClipboard #6598

Merged
merged 12 commits into from Oct 31, 2023

Conversation

CoderDake
Copy link
Contributor

@CoderDake CoderDake commented Oct 26, 2023

Adds a fallback behaviour for copying which will allow copy buttons to actually work in VSCode.

NOTE: success toasts will no longer show in VSCode though.

I've also added some logging just in case a user is looking at their console when a copy fails. Hopefully this provides enough information that the unhide-able error doesn't look suspicious.
Screenshot 2023-10-26 at 10 53 39 AM

Related to Dart-Code/Dart-Code#4814
Fixes #5775

@CoderDake CoderDake marked this pull request as ready for review October 26, 2023 15:28
@CoderDake CoderDake requested review from bkonyi and a team as code owners October 26, 2023 15:28
// This post message is only relevant in VSCode. If the parent frame is
// listening for this command, then it will attempt to copy the contents
// to the clipboard in the context of the parent frame.
window.parent?.postMessage(
Copy link
Contributor

@DanTup DanTup Oct 26, 2023

Choose a reason for hiding this comment

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

You can probably use the existing postMessage in packages\devtools_app\lib\src\shared\config_specific\post_message\post_message_web.dart to avoid needing multiple stub files for this API? (eg. have a single copy function that just calls the shared postMessage)?

Edit: Actually looks like the postMessage function there maybe isn't used... maybe it was only partly done? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

interesting, yeah it doesn't look like it exposed 🤔
Would you be alright with me just leaving the multi stub files for the copy behaviour?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems reasonable to me (if that's what's tested and works). It seems to be what the "launch URL" functionality (that I just re-discovered) does. Maybe it's worth adding a TODO that we could possibly refactor these to reduce the number of stubs though (both copy + launch url)?

(I suspect I added some of that dead postMessage code, so I'm happy to look, and either consolidate or delete the unused bits)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Issue created :)
#6613

@@ -0,0 +1,7 @@
// Copyright 2023 The Chromium Authors. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

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

remove the stub file and just use the desktop file in the conditional import

Comment on lines 5 to 6
// ignore: avoid_web_libraries_in_flutter, as designed
import 'dart:html';
Copy link
Member

Choose a reason for hiding this comment

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

please use package:web here. See #6607


if (successMessage != null) notificationService.push(successMessage);
} catch (e) {
_log.warning(
Copy link
Member

Choose a reason for hiding this comment

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

check if we are embedded and in VS code before doing any of this

Copy link
Member

Choose a reason for hiding this comment

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

You can check if we are embedded by using ideTheme.embed and you can check which IDE is hosting by checking here: https://github.com/flutter/devtools/blob/master/packages/devtools_app/lib/src/shared/analytics/_analytics_web.dart/#L733.

Although, I think we may want to modify the IdeTheme class to include the IDE name so we can look it up from there instead of having to look at our analytics file.

@CoderDake CoderDake added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 31, 2023
@auto-submit auto-submit bot merged commit e10051a into flutter:master Oct 31, 2023
23 checks passed
kenzieschmoll pushed a commit that referenced this pull request Nov 16, 2023
![](https://media.giphy.com/media/4URfklUToxk6A/giphy.gif)

Adds a fallback behaviour for copying which will allow copy buttons to actually work in VSCode.

NOTE: success toasts will no longer show in VSCode though.

I've also added some logging just in case a user is looking at their console when a copy fails. Hopefully this provides enough information that the unhide-able error doesn't look suspicious.
![Screenshot 2023-10-26 at 10 53 39 AM](https://github.com/flutter/devtools/assets/1386322/b4ebd139-f3f5-4dd6-8c62-794f7d6df80d)

Related to Dart-Code/Dart-Code#4814
Fixes #5775
kenzieschmoll pushed a commit that referenced this pull request Nov 17, 2023
![](https://media.giphy.com/media/4URfklUToxk6A/giphy.gif)

Adds a fallback behaviour for copying which will allow copy buttons to actually work in VSCode.

NOTE: success toasts will no longer show in VSCode though.

I've also added some logging just in case a user is looking at their console when a copy fails. Hopefully this provides enough information that the unhide-able error doesn't look suspicious.
![Screenshot 2023-10-26 at 10 53 39 AM](https://github.com/flutter/devtools/assets/1386322/b4ebd139-f3f5-4dd6-8c62-794f7d6df80d)

Related to Dart-Code/Dart-Code#4814
Fixes #5775

Mirror package:web version from Flutter SDK
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App
Projects
None yet
Development

Successfully merging this pull request may close these issues.

copy is not successful NotAllowedError: The Clipboard API has been blocked
4 participants