Clipboard Setting Issue #786 #790
Conversation
After testing an id of 0 had not caused any problems that I know of, but just to be safe I will change it to 999 Signed-off-by: aramrw <106574385+aramrw@users.noreply.github.com>
cashewnuttynuts
added
kind/enhancement
|
View Playwright Report (note: open the "playwright-report" artifact) |
Screencast.from.04-10-2024.03.00.20.PM.webmI don't usually use this feature, but it seems to already be working correctly for me? Has anyone reproduced this? |
Yomitan.mp4Maybe you could try with my yomitan settings? Either way there is something broken somewhere, but I think other people should test it as well 👍🏼 |
|
I'm unable to replicate this using those settings using the normal search page. But I can replicate it like this: Is there a way to open that special search page without having to enable |
| @@ -1303,7 +1327,7 @@ export class Backend { | |||
|
|
|||
| this._mecab.setEnabled(options.parsing.enableMecabParser && enabled); | |||
|
|
|||
| if (options.clipboard.enableBackgroundMonitor && enabled) { | |||
| if ((options.clipboard.enableBackgroundMonitor || options.clipboard.enableSearchPageMonitor) && enabled) { | |||
There was a problem hiding this comment.
This change seems fine and good but I'm not sure about the rest of this pr. Can't replicate the issue you're mentioning at all.
This bit "No longer creates a new search window everytime clipboard changes (unless background clipboard text monitoring is also enabled)." seems to already be how Yomitan works.
I would be okay with approving and getting this merged with only this one line changed. Maybe split the rest into a separate pr and try to provide some steps for reproducing the issue.
There was a problem hiding this comment.
This bit "No longer creates a new search window everytime clipboard changes (unless background clipboard text monitoring is also enabled)." seems to already be how Yomitan works.
That is actually not true. I just tested it by cloning and only adding the if statement part and it does in fact create a new popup every time with only the search page clipboard text monitoring being enabled, it's only supposed to update the search page if one is open, not create a new one (unless background clipboard text monitoring) is enabled.
Not only that, even with that if statement, background clipboard text monitoring does all the work, you don't even need to have search page clipboard text monitoring enabled for it to update the search page, so basically it's useless.
In my pr, the enable cb monitor if either cb setting === true commit does not disable making a brand new popup every time if one is not open (which is the main problem), it only give it permission to update the search without needing the first option to be enabled.
Is there a way to open that special search page without having to enable
Enable background clipboard text monitoringfirst?
Yes, under Native Keyboard Shortcuts -> Open the popup window. But that doesn't change anything, it still creates a new popup every time, because the logic in backend.js has no other choice. That's the whole reason I made the ghost tab.
The rest of my pr allows the second option, instead of creating a new search tab popup, to create a fake tab if none exist, or update the search page if one does.
I believe this was the intended feature of that second option..? However as I mentioned before, it currently does nothing...
I would be okay with approving and getting this merged with only this one line changed. Maybe split the rest into a separate pr and try to provide some steps for reproducing the issue.
As for this, there's not much else I can do on my end, If you have doubts I can go and get some more users to go upvote / give further evidence in the issue that I made.
As I showed in the video, I am on the latest version + chrome (tried right now, it still doesn't work for me), with default settings, and also by cloning this repo and only adding that if statement (default settings). Same issue, that video is still accurate as of right now.
I would like more developers to test this before my pr gets discarded, as I can't be the only person experiencing this issue, that doesn't make any sense.
I will try to get some regular users to try it and see what's going on for them as well. 👍🏼
There was a problem hiding this comment.
I just tested this again to sanity check and yeah just including this one line doesn't look like a good idea. I still can't replicate any of the other issues though.
I think the issue you're having needs a step by step reproduction process starting from Yomitan set to default settings.
There was a problem hiding this comment.
Steps
- Reset Yomitan settings.
- Only Turn on
search pageclipboard text monitoring.
- Copy a word to clipboard... -> Nothing happens.
- Turn off
search pageclipboard text monitoring. - Only turn on
backgroundclipboard text monitoring`.
- Copy a word to clipboard... -> Search popup gets updated.
Chrome Extension Video Details 5/27/2024.
- Yomitan
v24.5.14.1from Google Chrome Store (Default Settings). - Google Chrome
v125.0.6422.112(Official Build) (64 bit)
Also Tested on
- Microsoft Edge
v126.0.2592.13(Official Build) beta (64 bit)
Chrome Extension Video Evidence
Desktop.2024.05.27.-.15.online-video-cutter.com.mp4
- I also tested this by cloning the Github repo, it does the exact same thing shown in the Chrome Extension Video.
|
Thank you for the thorough explanation and example of the issue. I think I see what's going wrong here. Can you test if #1003 fixes the issues? I think you're fixing the wrong issue to fix your issue here. |
|
perfect thanks 👍🏼 that fixed it |
|
Closing in favor of #1003 |
Fixed #786
Regarding
search page clipboard text monitoringfeature changes.background clipboard text monitoringenabled for it to update the search window.background clipboard text monitoringis also enabled).Prop drilling a new boolean variable was the simplest/cleanest way I could implement this feature without refactoring.
This doesn't affect any apis or functions since
isShortcutcan be undefined.Regarding background/backend.js
1165 - 1184