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.

Sign up for GitHub

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

Jump to bottom

Fix for search not working with vte 0.6+ (issue #1752) #1769

Merged
merged 1 commit into from
Sep 8, 2021
Merged

Fix for search not working with vte 0.6+ (issue #1752) #1769

merged 1 commit into from
Sep 8, 2021

Conversation

dadukhin
Copy link
Contributor

@dadukhin dadukhin commented Jun 1, 2020

Fix for issue: #1752 Search not working anymore after vte libraries update to 0.60. As you can see from: https://lazka.github.io/pgi-docs/Vte-2.91/classes/Terminal.html#Vte.Terminal.search_set_gregex, this function is deprecated: "This function does nothing since version 0.60."

mlouielu
mlouielu requested changes Jul 10, 2020
View reviewed changes
Copy link
Collaborator

@mlouielu mlouielu left a comment

Choose a reason for hiding this comment

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

We can polish the flag setup for the Vte.Regex.new_for_search()

guake/boxes.py Outdated Show resolved Hide resolved
dadukhin
dadukhin commented Jul 16, 2020
View reviewed changes
Copy link
Contributor Author

@dadukhin dadukhin left a comment

Choose a reason for hiding this comment

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

added raw pcre2 flags. Should use REGEX_FLAGS_DEFAULT instead?

@scobby
Copy link

scobby commented Sep 1, 2020

any progress on this?

@mlouielu
Copy link
Collaborator

any progress on this?

Some polish need to applied, Vte.REGEX_FLAGS_DEFAULT should be used.

@mlouielu mlouielu mentioned this pull request Nov 10, 2020
Closed
@AurevoirXavier
Copy link

Need this update!

@dadukhin dadukhin requested a review from mlouielu November 15, 2020 20:39
@AurevoirXavier
Copy link

Any updates?

16 days.

@mlouielu
Copy link
Collaborator

mlouielu commented Dec 2, 2020 •
edited
Loading

Any updates?

16 days.

It is clear that this PR have mess with the master branch commit history and need to rebase or some what.

@AurevoirXavier, you are an Open source contributor, too. IMHO If you want to push something going on, this comment is not the best thing you can do.

@AurevoirXavier
Copy link

AurevoirXavier commented Dec 2, 2020 •
edited
Loading

Any updates?
16 days.

It is clear that this PR have mess with the master branch commit history and need to rebase or some what.

@AurevoirXavier, you are a Open source contributor, too. IMHO If you want to push something going on, this comment is not the best thing you can do.

But @dadukhin requested a review from you 16days ago and I can't see any feedback.

Your comment:

Some polish need to applied,

And I see him push those polish. But didn't get any feedback.

I'm not familiar with this project, if you already reviewed this PR and you think something wrong please request a change. So other people could push this.

@mlouielu
Copy link
Collaborator

mlouielu commented Dec 2, 2020

Any updates?
16 days.

It is clear that this PR have mess with the master branch commit history and need to rebase or some what.
@AurevoirXavier, you are a Open source contributor, too. IMHO If you want to push something going on, this comment is not the best thing you can do.

But @dadukhin requested a review from you 16days ago and I can't see any feedback.

Mate, not every one have time on open source stuff when they are busy.
If you have the ability to review PR, feel free to give the constructive feedback.

Or, if you really need this feature, you can build Guake by yourself. Code is here and it works.
You can refer to 32.16. Downloading Other’s Patches, to download this PR.

mlouielu
mlouielu requested changes Dec 2, 2020
View reviewed changes
Copy link
Collaborator

@mlouielu mlouielu left a comment

Choose a reason for hiding this comment

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

Somehow git commit history is mess, please clean it up by git rebase.

@dadukhin dadukhin force-pushed the master branch 2 times, most recently from dcb2157 to e2d8498 Compare December 5, 2020 09:46
@dadukhin dadukhin requested a review from mlouielu December 5, 2020 23:20
mlouielu
mlouielu requested changes Dec 7, 2020
View reviewed changes
Copy link
Collaborator

@mlouielu mlouielu left a comment

Choose a reason for hiding this comment

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

Minor changes need to apply, thanks

guake/boxes.py Outdated Show resolved Hide resolved
guake/guake_app.py Outdated Show resolved Hide resolved
@dadukhin dadukhin requested a review from mlouielu December 9, 2020 01:27
mlouielu
mlouielu previously approved these changes Dec 11, 2020
View reviewed changes
Copy link
Collaborator

@mlouielu mlouielu left a comment

Choose a reason for hiding this comment

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

Last with the commit message, please use uppercase Fix.

Otherwise LGTM, thanks @dadukhin for the contribution!

@dadukhin dadukhin requested a review from mlouielu December 13, 2020 01:27
mlouielu
mlouielu previously approved these changes Dec 22, 2020
View reviewed changes
Copy link
Collaborator

@mlouielu mlouielu left a comment

Choose a reason for hiding this comment

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

LGTM!

@cayacdev
Copy link

cayacdev commented Apr 8, 2021

Hi, what is the status about this PR? Seems that travis-ci is broken and doesn't report the status back to github.

The search feature doesn't work for me as well (VTE: 0.64.0). It would be very nice to have this feature soon ;)

@Davidy22 Davidy22 mentioned this pull request Sep 4, 2021
Closed
Davidy22
Davidy22 requested changes Sep 8, 2021
View reviewed changes
Copy link
Collaborator

@Davidy22 Davidy22 left a comment

Choose a reason for hiding this comment

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

Hello,

I ran this and it works great, so I'm mostly ready to merge. We've done some updates to fix our CI since this patch was created. so I just need you to do an update and make sure your changes pass CI. I've done some quick rough work in another repo and this commit contains all the changes you'll need to pass our build system so you can just grab that change and amend your commit with it. Once that's done I'll pull this and finally get this issue closed.

guake/boxes.py Outdated
self.searchre = GLib.Regex(text, 0, 0)
term.search_set_gregex(self.searchre, 0)

self.searchre = Vte.Regex.new_for_search(text, -1, Vte.REGEX_FLAGS_DEFAULT | PCRE2_MULTILINE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the only line that needs to be changed to pass CI, on my branch I changed it to:

            self.searchre = Vte.Regex.new_for_search(
                text, -1, Vte.REGEX_FLAGS_DEFAULT | PCRE2_MULTILINE
            )

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just went and did it myself.

@Davidy22 Davidy22 merged commit d88fc53 into Guake:master Sep 8, 2021
joebonrichie pushed a commit to solus-packages/guake that referenced this pull request Aug 14, 2023
@chax @JoshStrobl
Fix search functionality in Guake
87051a0 
Summary: This patch just fixes search functionality using this [PR](Guake/guake#1769) from upstream

Test Plan: Restart Guake and test search functionality

Reviewers: #triage_team, JoshStrobl

Reviewed By: #triage_team, JoshStrobl

Subscribers: JoshStrobl

Differential Revision: https://dev.getsol.us/D10678
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Davidy22 Davidy22 Davidy22 approved these changes

@mlouielu mlouielu mlouielu left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dadukhin @scobby @mlouielu @AurevoirXavier @cayacdev @Davidy22