Skip to content

Conversation

@danirabbit
Copy link
Member

Can be rebase merged if desired.

Cleanups:

  • Entries shouldn't use tooltip, they have placeholder for this purpose. Use the more descriptive placeholder
  • activate_entry is only called by the action so search text is never used
  • don't connect signals in a separate function. Let's us scope down variables
  • Organize and remove commented code

Absorb into mainwindow

  • We can't subclass SearchEntry in GTK4. We could put it in a bin, but it needs access to a lot of things in Window anyways, so this is much simpler than reaching across classes for access
  • Search entry can be private here, its not accessed outside this class

@danirabbit danirabbit requested a review from a team September 23, 2025 17:43
@stsdc
Copy link
Member

stsdc commented Sep 26, 2025

  • Tooltip: The tooltip was there basically to make a key combination discoverable. Are we sure, we want to remove it?
  • Search.vala : In the GTK4 PR the SearchEntry is wrapped in Gtk.Box (i know should be a Bin). But I'm still in favour of keeping it in a separate class to contain this filter func and other objects.

@danirabbit
Copy link
Member Author

danirabbit commented Sep 30, 2025

The tooltip was there basically to make a key combination discoverable. Are we sure, we want to remove it?

Yeah it's not expected for text entries or other labeled widgets to have tooltips. Tooltips should really only be for things that don't already have labels like image buttons

But I'm still in favour of keeping it in a separate class to contain this filter func and other objects

What do you think about moving the filter func etc into ProcessView? I think that would improve encapsulation. I really don't like how much has to be publicly exposed with the search.vala class

@stsdc
Copy link
Member

stsdc commented Oct 1, 2025

Yeah, I think this is a good solution. 👍

@stsdc stsdc merged commit 1a9e03a into main Oct 1, 2025
4 checks passed
@stsdc stsdc deleted the danirabbit/search-cleanups branch October 1, 2025 08:33
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