-
Notifications
You must be signed in to change notification settings - Fork 185
Add search capabilities to WebKit based Browser #2223
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.
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
Conversation
|
SWT snippet to test with: Problems that I see so far:
|
Test Results 545 files 545 suites 30m 17s ⏱️ Results for commit 6904ea2. ♻️ This comment has been updated with latest results. |
For that we need the option:
A native snippet works fine for this, so there must be something wrong in how we handle SWT events... Maybe its bad to finish the search on text modify events. GTK3+WebKit Snippet: (I'm not a 100% sure how the search is meant to be done in this case, but pressing buttons shows at last flashing search results) HTML: |
|
I've refactored the code and fixed the finish search logic. What remains is making the new dialog look good / behave well and the odd behavior of the search for some search strings. I'm trying to compile WebKit locally, to debug this... What we have installed doesn't have debug symbols for me to check what is going on during |
4c30cc6 to
1f27baa
Compare
|
Looks like I had to use Next we must decide how we want the dialog to behave/look, where to place it and so on. My main issues with the current change are:
|
|
The build job error so far is: I'm not sure what that means. |
|
Are you on top of master branch? If not, please rebase |
I think so, last commit I see is: I don't receive anything new if I pull from |
| }); | ||
| browser.addControlListener(ControlListener.controlResizedAdapter(e -> closeSearchDialog())); | ||
| browser.addControlListener(ControlListener.controlMovedAdapter(e -> closeSearchDialog())); | ||
| browser.getShell().addControlListener(shellMoveListener); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if there is a way to close the search shell without needing to add this listener to the browsers shell. The move listener on the browser will get notifications only if the browser moves within the parent... Not if the whole window moves.
Since the search shell is opened at a specific location, it looks odd when moving the browsers shell - the search shell remains where it was opened.
Yes, that is a must have, positioning at least. Currently if I use it inside IDE on a dynamic Help view opened on the right side of the screen, the search dialog is shown over Package Explorer bottom on the left side :-) At least it should be able to move / resize the dialog? Should we re-target the change to the JFace or even workbench code / platform UI? So we will add support for the I mean, we provide a callback interface that can be used by workbench parts to hook into the "show find dialog" trigger on SWT side and show default find dialog that we redirect to webkit API? So SWT uses "simple" find dialog by default if nothing is set, and if it is running from workbench views that supports the "better" dialog, if would use that? |
I don't know how this would work, any API we add will have to be at I agree, ideally we only add rudimentary search APIs, that can be used by a client. But I don't see how. Even with internal methods I'm not sure we will achieve anything in our own product... I've commented this on the Advantest ticket.
IMO best to try to get some SWT based dialog working. It should be enough for searching. |
db3f27b to
147f501
Compare
|
@iloveeclipse can you check the search dialog now? Here is my intent for its behavior:
The only thing that is hard-coded right now is the dialog ( We also have a problem with e.g. the JavaDoc hover - Ctrl+F on a focused JavaDoc hover shows the search dialogs and closes the JavaDoc hover... I don't know how we want to fix that. I am guessing searching should be explicitly enabled per Maybe we add some internal method " |
|
Only test fail is this one, from what I can tell: #1991 @iloveeclipse is on a vacation this week, I'm not sure if there is anyone else interested in this new addition to WebKit based browsers. So probably this change will wait for @iloveeclipse to review it. |
|
@iloveeclipse can you review this change? I'll think on how we can enable/disable the search per |
|
Unfortunately I had a crash after playing a bit with the new search. |
bundles/org.eclipse.swt/Eclipse SWT WebKit/gtk/org/eclipse/swt/browser/WebKit.java
Outdated
Show resolved
Hide resolved
|
I've fully removed locally created "move" cursor & rebased. |
|
Build is broken due #2200. |
This changes adds a search dialog to WebKit browsers. The search dialog is opened with Ctrl+F and has next/previous word matching capabilities. The search dialog is not available for Browsers in a Shell with the SWT.TOOL style. The search capabilities can be disabled with the following system property: -Dorg.eclipse.swt.internal.webkitgtk.disableBrowserSearch=true Fixes eclipse-platform#2222
|
Yet another unrelated issue: |
|
Finally! @trancexpress : please add N&N for the new feature on Linux. |
This changes adds a search dialog to WebKit browsers. The search dialog is opened with Ctrl+F and has next/previous word matching capabilities.
Fixes: #2222