Add extension for document-scoped search #294

loichuder · 2021-11-26T09:27:21Z

Well, that wasn't as easy as I first thought 😅

I could not reuse @jupyterlab/documentsearch-extension plugin directly as it relies on ILabShell to find the current searchable widget. I therefore rewrote the plugin for retrolab in a new @retrolab/documentsearch-extension to account for retrolab's differences.

I still have a small issue on RetroLab's landing page where Ctrl+F is caught when the focus is on the Files tab when it should not. This is perhaps related to the way I handled currentWidget in the new plugin.

EDIT: Now fixed !

Anyway, before going further, I wanted to draft this first functional implementation to know if I was going the right way. Any comments/feedback are welcomed.

github-actions · 2021-11-26T09:27:36Z

👈 Launch RetroLab on Binder

loichuder · 2021-11-26T09:28:24Z

packages/documentsearch-extension/src/index.ts

+    app: JupyterFrontEnd,
+    retroShell: IRetroShell,
+    registry: ISearchProviderRegistry
+  ) => {


Reimplementation of https://github.com/jupyterlab/jupyterlab/blob/a8dff39a1a2be850a80861e05145e763079b8794/packages/documentsearch-extension/src/index.ts#L38 for retrolab

loichuder · 2021-11-26T09:30:12Z

packages/documentsearch-extension/src/index.ts

+    // Apply the searchable class only to the active widget if it is actually
+    // searchable. Remove the searchable class from a widget when it's
+    // no longer active.
+    retroShell.currentChanged.connect((_, args) => {


Contrary to labShell.activeChanged, retroShell.currentChanged does not provide args containing the old and new widgets. I could not reproduce the behaviour in https://github.com/jupyterlab/jupyterlab/blob/a8dff39a1a2be850a80861e05145e763079b8794/packages/documentsearch-extension/src/index.ts#L64 where the class is removed for inactive widgets.

But, given that Retrolab only has one widget by browser tab, I think this is fine ? 🤷

jtpio · 2021-11-26T17:16:34Z

Thanks @loichuder for starting this!

I could not reuse @jupyterlab/documentsearch-extension plugin directly as it relies on ILabShell to find the current searchable widget. I therefore rewrote the plugin for retrolab in a new @retrolab/documentsearch-extension to account for retrolab's differences.

Right, this is something we often hit over the past couple of months when trying to reuse upstream plugins in alternate JupyterLab distributions.

Usually it's fine to duplicate the logic here as temporary solution, and in the meantime we can update the upstream plugin to make is more reusable. In this case this likely means making ILabShell optional.

jtpio · 2021-11-26T17:20:18Z

app/index.js

@@ -123,6 +124,9 @@ async function main() {
      ].includes(id)
    ),
    require('@jupyterlab/docprovider-extension'),
+    require('@jupyterlab/documentsearch-extension').default.filter(({ id }) =>
+      ['@jupyterlab/documentsearch:plugin'].includes(id)


It's good that we are at least able to reuse this plugin 👍

jtpio · 2021-11-26T17:22:51Z

UI Tests failure looks relevant, as the Edit menu now shows new entries:

So we can update the reference snapshots with the ones generated in https://github.com/jupyterlab/retrolab/actions/runs/1506929915

loichuder · 2021-11-29T13:03:28Z

In this case this likely means making ILabShell optional

Would it be possible that @jupyterlab/documentsearch-extension:labShellWidgetListener only requires a shell that implements the needed features (activeWidget and activeChanged) ? Then, we could reuse it by having IRetrolabShell match this signature.

In the meantime, I will keep the duplication as you said.

jtpio · 2021-11-30T10:01:00Z

Would it be possible that @jupyterlab/documentsearch-extension:labShellWidgetListener only requires a shell that implements the needed features (activeWidget and activeChanged)

Maybe, or this could be hoisted to the JupyterFrontEnd.IShell base interface although for now we would like to not increase the API surface of JupyterFrontEnd.IShell.

loichuder · 2021-12-06T09:31:40Z

@jtpio This is now ready for review.

Could you explain how to update the screenshots ? Perhaps we could also add this information to CONTRIBUTING if that's something maintainers can do ?

jtpio · 2021-12-06T14:01:17Z

Could you explain how to update the screenshots ? Perhaps we could also add this information to CONTRIBUTING if that's something maintainers can do ?

Yes absolutely. For reference there is some documentation in another repo: https://github.com/jupyterlite/jupyterlite/blob/main/CONTRIBUTING.md#reference-image-captures

But we should have something similar here too. Or as an alternative in the JupyterLab contribution docs and we link to it from here: https://github.com/jupyterlab/jupyterlab/blob/master/docs/source/developer/contributing.rst#visual-regression-and-ui-tests

loichuder · 2021-12-08T08:27:23Z

CI passed 🎉

jtpio · 2021-12-08T09:41:02Z

Thanks!

This looks good on Binder:

search-replace.mp4

We can document updating the reference snapshots in a separate PR.

loichuder · 2021-12-08T09:42:47Z

We can document updating the reference snapshots in a separate PR.

Yes that was my plan 👍

jtpio · 2021-12-08T09:44:16Z

cc @yuvipanda if you have any input on this.

I remember you raised some concerns on apps overriding default browser keyboard shortcuts. Here that would be Ctrl / Cmd - F

If this comes to much in the way, we could also make it an opt-in instead, for example via a PageConfig option.

yuvipanda · 2021-12-08T11:03:27Z

Thank you very much for the ping, @jtpio! I really appreciate it! I am also really grateful you working on this, @loichuder!

The browser's default Cmd+F is very helpful, as it matches the mental model of 'search for any text I can see' - including all the UI elements. This is very helpful in cases where the user is trying to follow instructions like 'Find item X' - this works even when you just have menu items open (for example):

IMO we shouldn't override default browser functionality. The find / replace stuff is pretty cool, but I'd suggest it not be bound to Cmd-F by default. It can be accessed from the menu (as you can in classic notebook right now), and a PageConfig can make that binding if necessary.

loichuder · 2021-12-08T11:36:14Z

IMO we shouldn't override default browser functionality. The find / replace stuff is pretty cool, but I'd suggest it not be bound to Cmd-F by default.

Make sense to me.

As I understand, the keyboard shortcut is set via JupyterLab's document-search extension as I reuse one of the plugins of the extension in my implementation.

Is this true ? If so, what would be the way to override this ?

jtpio · 2021-12-08T13:55:23Z

Normally we should be able to disable the shortcut here in retro.

For example when testing this PR on Binder:

search-replace-keybinding.mp4

Which means we could put the following snippet:

{
    "shortcuts": [
        {
            "command": "documentsearch:start",
            "keys": [
                "Accel F"
            ],
            "selector": ".jp-mod-searchable",
            "disabled": true
        }
    ]
}

In the retro settings by default.

Reusing the settings override system added in #289 so we can customize jupyter.lab.shortcuts.

jtpio · 2021-12-08T14:36:30Z

Maybe this could be implemented as a separate plugin with its own settings similar to the menu plugin:

retrolab/packages/application-extension/src/index.ts

Lines 199 to 229 in 49b9ae3

    
           /** 
        
            * A plugin to customize menus 
        
            * 
        
            * TODO: use this plugin to customize the menu items and their order 
        
            */ 
        
           const menus: JupyterFrontEndPlugin<void> = { 
        
             id: '@retrolab/application-extension:menus', 
        
             requires: [IMainMenu], 
        
             autoStart: true, 
        
             activate: (app: JupyterFrontEnd, menu: IMainMenu) => { 
        
               // always disable the Tabs menu 
        
               menu.tabsMenu.dispose(); 
        
               const page = PageConfig.getOption('retroPage'); 
        
               switch (page) { 
        
                 case 'consoles': 
        
                 case 'terminals': 
        
                 case 'tree': 
        
                   menu.editMenu.dispose(); 
        
                   menu.kernelMenu.dispose(); 
        
                   menu.runMenu.dispose(); 
        
                   break; 
        
                 case 'edit': 
        
                   menu.kernelMenu.dispose(); 
        
                   menu.runMenu.dispose(); 
        
                   break; 
        
                 default: 
        
                   break; 
        
               } 
        
             } 
        
           };

So it could be disabled with jupyter labextension disable by end users.

jtpio · 2021-12-09T17:09:14Z

Also when we implement the override for the disabled keyboard shortcut, we could also add a UI test to make sure updating to future versions of @jupyterlab packages doesn't break this.

For example this could mean using the await page.keyboard.press('Control+F) method as documented here: https://playwright.dev/docs/api/class-keyboard/
And making sure the search / replace popup is not displayed.

It would be fine to add that test in a follow-up PR if we want to merge this PR earlier.

loichuder · 2021-12-13T07:21:03Z

It would be fine to add that test in a follow-up PR if we want to merge this PR earlier.

As disabling Ctrl+F is the behaviour we want to have, I think it is better to add it to this PR. I just need to make the override work 😄

loichuder · 2021-12-21T16:13:28Z

Sorry it took me a while to find the courage to navigate once again in the treacherous plugin sea.

I added a plugin to disable the shortcut: 4aed598. Not sure if that is the way you intended it @jtpio so don't hesitate to direct me towards another solution. I will work on the UI test, next.

Funfact: While testing, I noticed that the Ctrl+F event is already caught for text files like MD, making a custom search bar appear:

This comes not from the documentsearch-extension but from the notebook classic !

So even by disabling the documentsearch-extension shortcut, the only way to get the browser search for text files is to click out from the document before hitting Ctrl+F.

jtpio · 2021-12-23T11:40:29Z

I added a plugin to disable the shortcut: 4aed598. Not sure if that is the way you intended it @jtpio so don't hesitate to direct me towards another solution. I will work on the UI test, next.

Thanks @loichuder. I was more thinking about using the declarative way and override the shortcut in the schema file with jupyter.lab.shortcuts. Although I haven't tested yet and the end result is likely to be the same.

Funfact: While testing, I noticed that the Ctrl+F event is already caught for text files like MD, making a custom search bar appear:

Probably this one would be fine to keep as is yes, especially since classic behaves the same.

jtpio · 2021-12-24T14:26:41Z

FYI @loichuder I pushed the following commit to switch to the declarative approach: 55915e6

And rebased to grab the changes from #309. Will follow-up with the snapshots update if needed.

jtpio · 2021-12-24T15:05:47Z

switch to the declarative approach

Looks like this is actually picked up by JupyterLab too, which means that the shortcut is now disabled in JupyterLab too:

I wouldn't have expected that jupyter.lab.shortcuts configuration to be picked up if the plugin is not loaded on the page. Gonna check whether this is an issue in lab, or if we can find a workaround here in the meantime.

jtpio · 2021-12-28T11:10:49Z

I wouldn't have expected that jupyter.lab.shortcuts configuration to be picked up if the plugin is not loaded on the page. Gonna check whether this is an issue in lab, or if we can find a workaround here in the meantime.

This is now tracked in jupyterlab/jupyterlab#11754.

Based on these observations, the least surprising for the end user would then be to keep the programmatic shortcut override added in 38a6166. So it doesn't have any side-effect in JupyterLab.

I'll update this PR to revert the last two commits, so we can get it out in the next release and revisit later depending on the status of jupyterlab/jupyterlab#11754.

jtpio · 2021-12-28T12:14:11Z

Folks can use the Advanced Settings editor to re-enable the shortcut if they would like to (opt-in):

enable-crtl-f.mp4

jtpio

Thanks!

loichuder · 2022-01-04T07:49:58Z

Thanks for following that up and ironing out the last wrinkles 🙂 👍

jtpio · 2022-01-04T08:16:42Z

@loichuder thank you for working on this!

FYI this change was released in 0.3.14: https://github.com/jupyterlab/retrolab/releases/tag/v0.3.14

loichuder commented Nov 26, 2021

View reviewed changes

jtpio added the enhancement New feature or request label Nov 26, 2021

jtpio reviewed Nov 26, 2021

View reviewed changes

loichuder force-pushed the doc-search branch from 456d6f8 to 0968528 Compare December 6, 2021 09:28

loichuder marked this pull request as ready for review December 6, 2021 09:30

jtpio added this to the 0.3.x milestone Dec 8, 2021

loichuder force-pushed the doc-search branch from c9b9526 to 4aed598 Compare December 21, 2021 16:04

loichuder and others added 4 commits December 24, 2021 15:20

Add extension for document-scoped search

656c09f

Updated snapshots for UI tests

5f24af6

Add a plugin to disable document search shortcut

38a6166

Switch to declarative keyboard shortcuts

55915e6

jtpio force-pushed the doc-search branch from 4aed598 to 55915e6 Compare December 24, 2021 14:21

Update reference snapshots

37b0c2e

jtpio mentioned this pull request Dec 28, 2021

Check the application includes the plugin and is activated before reconciling settings jupyterlab/jupyterlab#11754

Closed

Switch back to shortcut override plugin

7c0b058

jtpio force-pushed the doc-search branch from ca2e8fd to 7c0b058 Compare December 28, 2021 12:08

jtpio approved these changes Dec 28, 2021

View reviewed changes

jtpio merged commit ec926de into jupyterlab:main Dec 28, 2021

jtpio mentioned this pull request Jan 3, 2022

Blockers for using retrolab in data8 berkeley-dsep-infra/datahub#2422

Open

9 tasks

loichuder deleted the doc-search branch January 4, 2022 07:50

jtpio mentioned this pull request Apr 5, 2023

Misc search and replace issues in notebook 7 jupyter/notebook#6821

Closed

jtpio mentioned this pull request May 31, 2023

Find text only searches part of file jupyter/notebook#6697

Closed

jtpio mentioned this pull request Jun 8, 2023

Fix for "find searches only part of the file" bug jupyter/notebook#6905

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add extension for document-scoped search #294

Add extension for document-scoped search #294

loichuder commented Nov 26, 2021 •

edited

Loading

github-actions bot commented Nov 26, 2021

loichuder Nov 26, 2021

loichuder Nov 26, 2021

jtpio commented Nov 26, 2021

jtpio Nov 26, 2021

jtpio commented Nov 26, 2021

loichuder commented Nov 29, 2021

jtpio commented Nov 30, 2021

loichuder commented Dec 6, 2021

jtpio commented Dec 6, 2021

loichuder commented Dec 8, 2021

jtpio commented Dec 8, 2021

loichuder commented Dec 8, 2021

jtpio commented Dec 8, 2021

yuvipanda commented Dec 8, 2021

loichuder commented Dec 8, 2021

jtpio commented Dec 8, 2021

jtpio commented Dec 8, 2021

jtpio commented Dec 9, 2021 •

edited

Loading

loichuder commented Dec 13, 2021 •

edited

Loading

loichuder commented Dec 21, 2021 •

edited

Loading

jtpio commented Dec 23, 2021

jtpio commented Dec 24, 2021

jtpio commented Dec 24, 2021

jtpio commented Dec 28, 2021

jtpio commented Dec 28, 2021

jtpio left a comment

loichuder commented Jan 4, 2022

jtpio commented Jan 4, 2022

Add extension for document-scoped search #294

Add extension for document-scoped search #294

Conversation

loichuder commented Nov 26, 2021 • edited Loading

github-actions bot commented Nov 26, 2021

loichuder Nov 26, 2021

Choose a reason for hiding this comment

loichuder Nov 26, 2021

Choose a reason for hiding this comment

jtpio commented Nov 26, 2021

jtpio Nov 26, 2021

Choose a reason for hiding this comment

jtpio commented Nov 26, 2021

loichuder commented Nov 29, 2021

jtpio commented Nov 30, 2021

loichuder commented Dec 6, 2021

jtpio commented Dec 6, 2021

loichuder commented Dec 8, 2021

jtpio commented Dec 8, 2021

loichuder commented Dec 8, 2021

jtpio commented Dec 8, 2021

yuvipanda commented Dec 8, 2021

loichuder commented Dec 8, 2021

jtpio commented Dec 8, 2021

jtpio commented Dec 8, 2021

jtpio commented Dec 9, 2021 • edited Loading

loichuder commented Dec 13, 2021 • edited Loading

loichuder commented Dec 21, 2021 • edited Loading

jtpio commented Dec 23, 2021

jtpio commented Dec 24, 2021

jtpio commented Dec 24, 2021

jtpio commented Dec 28, 2021

jtpio commented Dec 28, 2021

jtpio left a comment

Choose a reason for hiding this comment

loichuder commented Jan 4, 2022

jtpio commented Jan 4, 2022

loichuder commented Nov 26, 2021 •

edited

Loading

jtpio commented Dec 9, 2021 •

edited

Loading

loichuder commented Dec 13, 2021 •

edited

Loading

loichuder commented Dec 21, 2021 •

edited

Loading