Skip to content

Conversation

@UrtsiSantsi
Copy link
Contributor

No description provided.

@UrtsiSantsi UrtsiSantsi requested a review from sonnyp as a code owner December 18, 2023 12:40
@sonnyp
Copy link
Contributor

sonnyp commented Dec 18, 2023

Since this is a continuation of #364

it would be nice to mention it in the PR description
and important to base your branch on that one so @SoNiC-HeRE is credited via his commits

It's a bit late/difficult to do now so before merging, I will make sure you're both credited as author of the commit.

@UrtsiSantsi
Copy link
Contributor Author

UrtsiSantsi commented Dec 18, 2023

I didn't use #364 directly - I copied some code from the demo, and I also used Text Editor and other code as reference. But @SoNiC-HeRE should be credited for sure - should I amend the commit message?

@UrtsiSantsi
Copy link
Contributor Author

There is a problem with this branch:

  1. Open new instance
  2. Open "Advanced Buttons"
  3. Close it
  4. Close (quickly) the library window
(re.sonny.Workbench.Devel:5): Gjs-CRITICAL **: 09:45:36.925: Attempting to call back into JSAPI during the sweeping phase of GC. This is most likely caused by not destroying a Clutter actor or Gtk+ widget with ::destroy signals connected, but can also be caused by using the destroy(), dispose(), or remove() vfuncs. Because it would crash the application, it has been blocked and the JS callback not invoked.
The offending signal was notify on GtkText 0x5649a419be60.
== Stack trace for context 0x5649a190bbb0 ==

It can be reproduced with other demos too as long as there are empty windows that can be searched, but they should be empty on start. It also seems to be time dependent.

I'm not sure how to handle this.

Copy link
Contributor

@sonnyp sonnyp left a comment

Choose a reason for hiding this comment

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

Works great, well done. Sorry for the delay.

I can't reproduce the issue you mentioned but see https://gitlab.gnome.org/GNOME/gjs/-/issues/327

Coul you add the Ctrl+F keyboard shortcut to the keyboard shortcut window?

using Gtk 4.0;

template $Search: Revealer {
transition-type: slide_up;
Copy link
Contributor

Choose a reason for hiding this comment

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

strange - besides this being set to slide_up - it endups being slide down (the default)

Screenshot from 2024-01-06 02-34-15

the animation looks weird - it should really be slide up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works fine for me even after the merge.


Button button_previous {
icon-name: "up";
tooltip-text: "Move to previous match (Ctrl+Shift+G)";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
tooltip-text: "Move to previous match (Ctrl+Shift+G)";
tooltip-text: _("Move to previous match (Ctrl+Shift+G)");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


Button button_next {
icon-name: "down";
tooltip-text: "Move to next match (Ctrl+G)";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
tooltip-text: "Move to next match (Ctrl+G)";
tooltip-text: _("Move to next match (Ctrl+G)");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@sonnyp
Copy link
Contributor

sonnyp commented Jan 6, 2024

Screenshot from 2024-01-06 12-46-47

The buttons should be flat and there should be a close button like Builder

@UrtsiSantsi
Copy link
Contributor Author

I modeled it after Text Editor, since it was created later and is supposedly more modern and in line with recent design:

image

As for the close buttons - we already discussed it and decided that using ESC is fine for now. I may add a breakpoint to show\hide the close button depending on the available space later on.

@UrtsiSantsi
Copy link
Contributor Author

I added the shortcut to the shortcut window, although it is not a global shortcut.
I can still reproduce the critical and I have some ideas how to fix it. I'll look into it maybe next week.

@UrtsiSantsi
Copy link
Contributor Author

Can we merge this and polish it later or do there is something missing?

@UrtsiSantsi UrtsiSantsi requested a review from sonnyp January 8, 2024 00:26
@sonnyp
Copy link
Contributor

sonnyp commented Jan 9, 2024

Can we merge this and polish it later or do there is something missing?

I'm concerned about #853 (comment)

But we can proceed and ship this "hidden" to polish. I made some tweaks e9a6b5e

@sonnyp sonnyp merged commit 67c9d5b into main Jan 10, 2024
@sonnyp sonnyp deleted the search2 branch January 10, 2024 17:50
This was referenced Jan 14, 2024
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.

3 participants