Made options to hide search bar, private mode, clear, and/or settings buttons & fixed Paste on Select bug. by JaredTweed · Pull Request #534 · Tudmotu/gnome-shell-extension-clipboard-indicator

JaredTweed · 2025-05-08T07:09:41Z

This pull request is built on top of my other pull request.

This ensures that:

Text always pastes.
The text that pastes is always the selected item, and never accidentally pastes the previously selected item.

Both of those bugs were frequent especially on slower hardware, but I think I entirely fixed those errors.

… buttons. Also made pressing Enter and clicking on an item have consistent behaviour. Fixed glitch where Paste on Select only worked when pressing Enter, and not when clicking.

… my version but forgot to include them in the commit), and fixed whitespace edits

…. Hopefully eliminates unsuccessful pasting attempts.

Tudmotu · 2025-09-21T07:10:01Z

Hi @JaredTweed,
I appreciate your intention of improving this extension with your PRs.
Can you please explain:

What is Meta.Laters and why should we use it over GLib?
Do you understand why the entry selection was made with delay in the first place?
Can the pasting fix be split in to a separate PR, and if not, why?
Additionally, can you explain the scenario in which pasting doesn't work properly?For example this was pasted with paste-on-select

You claimed in other PRs the extension "works better" with these changes, but I am a very heavy user of this extension and I rarely encounter any issues. In addition, other than the paste-on-select, none of the changes seem meaningful (some seem detrimental). Can you please elaborate on the issues you encountered?

JaredTweed · 2025-09-21T22:06:58Z

Q1) Meta.Laters is Mutter’s frame-synchronized scheduler. It runs callbacks at compositor phases like AFTER_REDRAW, which is ideal for preview image swaps and avoids UI races that idle/timers can’t. The code falls back to GLib.idle_add() if Meta.Laters isn’t present, keeping compatibility across versions.

Q2) Yes, the original delay buffered against focus and relayout churn while the menu opened. In #529 the focus delay is preserved using GLib.idle_add() so it doesn’t fight Shell’s animation, but the separate selection delay is removed and replaced with synchronous selection. This prevents the paste-ordering races caused by timers, and #534 then fully replaces them with event-driven sequencing.

Q3) The pasting fix can not be split, because the way it loads the menu causes it to load differently, causing some issues in the timers and the order in which they load things. #529 introduced the unified activation path and the _pastePending/owner-change hooks. #534 builds on this by replacing the last timeouts with sequential, signal-driven logic and fixing the “previous vs current selection” problem. While it could be split, #534 is a direct continuation of #529 and simpler to test together.

Q4) I had it on my last computer (a very old slow computer), where it would occasionally paste the second most recent instead of the most recent code (only when I copied large sections of text. Paste-on-select also failed when clicking an entry, because the paste is attempted before the clipboard owner switches and before focus returns to the target app. With #529 and #534, activation is unified: the menu closes first to return focus, the clipboard is updated, and the paste only happens once the owner-change signal is seen. This guarantees correct ordering and consistent behavior for both click and Enter. On my newer computer I don't have that issue.

JaredTweed · 2025-09-21T22:09:19Z

The other benefit of making it run sequentially, is that it will be more reliable, and not rely on specific animation/loading speeds.

JaredTweed · 2025-09-21T22:55:48Z

If you want, I can modify PR #529 to evade the same bugs as #534, but avoid using meta.laters instead of glib (so as to keep the change minimal). Would you like me to do that? I could get that done by tomorrow night. I just think it will be less reliable for future updates if we don't make this change.

Tudmotu · 2025-09-22T00:02:55Z

The delay has nothing to do with animations or layouts. You can see the delay is only used when the "move item to top" setting is enabled. This is because otherwise, iterating through the list of items via shortcuts becomes impossible: every time you select an entry it moves to the top, you select the next entry it moves to the top, the previous entry is now in place 2, you move to the next item and it's actually the previous entry, so you are effectively stuck in a loop between 2 entries. This delay should not be removed as it is crucial.

This PR has some issues which makes me uncomfortable merging it. I am leaving a review, but I highly suggest that you try first submitting a smaller PR, that does not include any fundamental changes to the way this extension works. Thousands of people if not more use it every day, and if it was this broken then I'd hear about it.

The scenario you are describing with the paste-on-select window focus race condition might present an actual issue. If that's the case, I suggest you find a reproducible example. I think maybe a moving the paste action to the "activate" handler is all that is needed (not sure about it). In any case, for me to be able to test a fix like that, I need to be able to first reproduce the underlying issue.

I know you put a lot of effort into this PR and I don't want to discourage you. But I have to consider the many many many users of this extension when I merge PRs, especially ones that make fundamental changes to the code. Thank you.

extension.js

JaredTweed · 2025-09-22T19:34:20Z

Okay, I did all the changes I found to be necessary and did a thorough bug search as well. Is this code sufficient? You can ignore PR #529 now and just pull this instead.

JaredTweed · 2025-09-22T19:34:35Z

Sorry for not checking it better before.

JaredTweed · 2025-09-22T20:15:41Z

This is the smallest I know how to make this PR, I am sorry. Thank you so much for the work you have put into this project!! I really appreciate your time.

constants.js

JaredTweed · 2025-09-23T18:28:43Z

@Tudmotu, I made the changes as clean as possible for you. Is it clean enough to be added to the extension?

JaredTweed · 2025-09-23T21:35:38Z

@Tudmotu I made the PR much smaller now, and kept your version of the timeouts that you like so that there is no longer that risk of making it create breaking changes. I also updated the code so that it has no conflicts with the base and can now be merged with the click of a button. I hope these additions are sufficient.

…items

JaredTweed · 2025-09-23T21:59:11Z

@Tudmotu I also got rid of all that Meta.Laters stuff and all those unnecessary changes that I made before

Tudmotu

I left another review. This PR is still too much of a mess. Follow the comments I left you and I will review it again afterwards. I'm not sure if you are using claude or if it's you that is adding all these unnecessary changes, but please stop. If you are using an agent to help, you need to double review its changes and make sure everything works properly.

One more thing: DO NOT resolve comments on your own. Only the reviewer should resolve comments. Resolving comments others leave makes it harder to see if you actually implemented the proper changes. This is basic open source etiquette.

Tudmotu · 2025-09-24T09:34:18Z

extension.js

+                if (currentlySelected && currentlySelected.entry)
+                    this.#updateClipboard(currentlySelected.entry);


In what situation is currentlySelected not defined?

Tudmotu · 2025-09-24T09:36:06Z

prefs.js

+        // Toggleable UI rows
+        this.field_show_search_bar = new Adw.SwitchRow({title: _("Show Search Bar")});
+        this.ui.add(this.field_show_search_bar);
+        this.field_show_private_mode = new Adw.SwitchRow({title: _("Show Private Mode")});
+        this.ui.add(this.field_show_private_mode);
+        this.field_show_settings_button = new Adw.SwitchRow({title: _("Show Settings Button")});
+        this.ui.add(this.field_show_settings_button);
+        this.field_show_clear_history_button = new Adw.SwitchRow({title: _("Show Clear History Button")});
+        this.ui.add(this.field_show_clear_history_button);
+


Why do you create the fields and add them here instead of following the pattern used throughout the rest of the file where the fields are created on top and are only attached here?

Tudmotu · 2025-09-24T09:37:38Z

extension.js

-            if (ENABLE_KEYBINDING)
-                that._bindShortcuts();
-            else
-                that._unbindShortcuts();
+            if (ENABLE_KEYBINDING) that._bindShortcuts();
+            else that._unbindShortcuts();


Meaningless whitespace changes like these clutter your PR. Please undo this change unless it is meaningful somehow. Avoid any changes that do not affect functionality.

Tudmotu · 2025-09-24T09:38:59Z

extension.js

+
+        // NEW toggles


What does "NEW" mean here? If someone sees this code in 3 years, would these still be "NEW"?
Please remove this comment and just maintain the same code style and patterns of the surrounding code.

Tudmotu · 2025-09-24T09:45:59Z

extension.js

                    this.#selectNextMenuItem(menuItem);
                    this._removeEntry(menuItem, 'delete');
-                    break;
+                    return Clutter.EVENT_STOP;


Why do you need to return these EVENT_STOP consts?

I think we may want to return Clutter.EVENT_STOP to prevent the event from propagating up the actor hierarchy — otherwise GNOME Shell may also handle it (e.g., close the popup or trigger other UI reactions).

Tudmotu · 2025-09-24T09:49:53Z

extension.js

+        // Ensure MOVE_ITEM_FIRST also applies when PASTE_ON_SELECT fast-path skips _refreshIndicator()
+        if (PASTE_ON_SELECT && MOVE_ITEM_FIRST && !menuItem.entry.isFavorite()) {
+            this._moveItemFirst(menuItem);
+        }


Why do you need to manually move the item here?

Tudmotu · 2025-09-24T09:50:39Z

extension.js

+        menuItem.icoBtn.visible = DELETE_ENABLED;
+        menuItem.deletePressId = icoBtn.connect('clicked', () => {
+            if (!DELETE_ENABLED) return;
+            this._removeEntry(menuItem, 'delete');
+        });


Why did you re-enable support for this feature? It's long gone as far as I can see.

Tudmotu · 2025-09-24T09:52:26Z

extension.js

-        // Create menu sections for items
-        // Favorites
+        // Favorites section


Undo every unnecessary change. I will not accept a PR that has any unnecessary changes. Keep you PR as clean as possible and adhere to the style and patterns used in the existing code.

Tudmotu · 2025-09-24T09:53:00Z

extension.js


+// NEW: toggleable UI elements


Remove the "NEW" comment here as well.

dynobo · 2025-09-24T10:37:19Z

@Tudmotu, as a grateful user of your extension who curiously follows this topic here (as I like the idea), I just want to take the opportunity to thank you for insisting on high quality standards and appropriate workflows. 👍
A clipboard manager deals with quite sensitive information, and the way you handle this here increases my trust in the extension even further! Thank you for your great work! ❤️

JaredTweed · 2025-09-24T15:47:08Z

@Tudmotu I will be more careful with the changes. Sorry about resolving the comments. I will let you resolve those. Thank you so much for being willing to take the time to review my code, I really appreciate your discipline in that. I really hope my contributions make it into the final product. Let me know if you would like any more changes.

…toggle it off

JaredTweed · 2025-09-26T16:49:12Z

@Tudmotu Thank you for your patience with me. Just so you know, I am ready for you to review it!

JaredTweed added 6 commits April 2, 2025 13:46

Made options to hide search bar, private mode, clear, and/or settings…

a84d82f

… buttons. Also made pressing Enter and clicking on an item have consistent behaviour. Fixed glitch where Paste on Select only worked when pressing Enter, and not when clicking.

fixed issue where the elements weren't updating properly (had them in…

6323206

… my version but forgot to include them in the commit), and fixed whitespace edits

reduced whitespace edits

8b5a0f3

Made pasting use functions sequentially rather than relying on timers…

a73da70

…. Hopefully eliminates unsuccessful pasting attempts.

Fixed paste to use the current selection rather than the previous one.

f1c0fd6

fixed screenshot previews to work again

7110102

This was referenced Jul 11, 2025

Made options to hide search bar, private mode, clear, and/or settings buttons & fixed Paste on Select bug. #529

Closed

"Paste on select" not working properly #543

Open

JaredTweed changed the title ~~Reorganized the code instead of using timeouts~~ Made the code sequential instead of using timeouts Aug 13, 2025

Merge branch 'Tudmotu:master' into clear-timeouts

7940a34

Tudmotu requested changes Sep 22, 2025

View reviewed changes

JaredTweed added 2 commits September 22, 2025 13:56

made updates in response to comments

8d454a7

did a thorough bug search

c94ee07

JaredTweed changed the title ~~Made the code sequential instead of using timeouts~~ Made options to hide search bar, private mode, clear, and/or settings buttons & fixed Paste on Select bug. Sep 22, 2025

hide empty message after copying items

36ab280

JaredTweed mentioned this pull request Sep 22, 2025

"Paste On Select" Bug #516

Open

dynobo reviewed Sep 23, 2025

View reviewed changes

constants.js Show resolved Hide resolved

JaredTweed added 5 commits September 23, 2025 09:57

kept old PR formatting

d17cc64

made formatting more similar to old code

abcaa48

made formatting more similar to old code

cbfe0d6

made #selectNextMenuItem keep old formatting

44a3488

made formatting more consistent

ab64e86

JaredTweed added 3 commits September 23, 2025 11:19

made formatting more similar

978b70b

fixed formatting

9ff7667

fixed 'MOVE_ITEM_FIRST' for paste on select

173d01d

JaredTweed added 5 commits September 23, 2025 16:07

merged upstream changes

9e2fc15

made changes more minimal

4369705

made diff smaller for PR

482d627

made diff smaller to see changes easier

33a2171

made the commit smaller

845689e

made it show only settings and private button when there are no menu …

8894ef4

…items

JaredTweed requested a review from Tudmotu September 23, 2025 22:11

Tudmotu requested changes Sep 24, 2025

View reviewed changes

JaredTweed added 8 commits September 24, 2025 09:39

cleaning code

65d1328

cleaning code more

a2da08a

cleaning code some more

cc81704

some more cleaning

1bae3ef

cleaned code

714d411

cleaning

aefec0c

removed support for 'DELETE_ENABLED'

a7a24c4

cleaned code

8568a99

JaredTweed requested a review from Tudmotu September 24, 2025 15:47

JaredTweed added 4 commits September 24, 2025 13:40

fixed error associated when turning on private mode

5901a8a

made toggle move properly

b8ba8ec

fixed glitch where private mode would be toggled on and you couldn't …

8ec48a4

…toggle it off

cleaned pr

020094d

		if (currentlySelected && currentlySelected.entry)
		this.#updateClipboard(currentlySelected.entry);

Conversation

JaredTweed commented May 8, 2025

Uh oh!

Tudmotu commented Sep 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JaredTweed commented Sep 21, 2025

Uh oh!

JaredTweed commented Sep 21, 2025

Uh oh!

JaredTweed commented Sep 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tudmotu commented Sep 22, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JaredTweed commented Sep 22, 2025

Uh oh!

JaredTweed commented Sep 22, 2025

Uh oh!

JaredTweed commented Sep 22, 2025

Uh oh!

Uh oh!

JaredTweed commented Sep 23, 2025

Uh oh!

JaredTweed commented Sep 23, 2025

Uh oh!

JaredTweed commented Sep 23, 2025

Uh oh!

Tudmotu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dynobo commented Sep 24, 2025

Uh oh!

JaredTweed commented Sep 24, 2025

Uh oh!

JaredTweed commented Sep 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Tudmotu commented Sep 21, 2025 •

edited

Loading

JaredTweed commented Sep 21, 2025 •

edited

Loading

Tudmotu left a comment •

edited

Loading