Add autocomplete field

[ghost]

This adds a new widget (subclass of Field) that provides the ability to specify a list of items to autocomplete from.

[frmdstryr]

Looks good to me. Nice work!

Copyright year should be updated and added to the header of the qt file.

[MatthieuDartiailh]

We also need a changelog entry. I will do my best to review this but it likely won't happen before next Wednesday at best.

[sccolbert]

Nice work! Looks like how I would have written it.

[MatthieuDartiailh]

Nice to know @sccolbert . I was wondering myself if having something built like Menu and that could be attached to any test like widget would not be better. WDYT ?

[frmdstryr]

Do the completer and model need to be deleted in destroy? Or does the gc handle that?

[bburan]

@MatthieuDartiailh I had not considered the use-case of other fields (e.g., HTML and Multiline). I guess one other advantage to attaching an AutoComplete item to any text widget is that we can make it easy subclass the Enaml AutoComplete widget to provide other means for auto-complete (e.g., database queries, etc.). When I get a chance, I'll put together a proof of concept.

[ghost]

@MatthieuDartiailh After further review, I don't think it's a good idea to try to implement a general Autocomplete that can attach to any text-based widget. For example, when I started stubbing out some code to implement the completion for the multiline field, I realized that we'd have to intercept key press events to determine when to show an auto-complete, add some code to figure out the current word under the cursor, and then show the autocomplete. It's a doable problem, but the implementation would be different enough from that for a regular single-line text field that we might as well create a MultilineAutocompleteField instead of trying to attach an Autocomplete. Plus what happens if we try to attach an Autocomplete to a non-text widget?

[ghost]

@frmdstryr I'm not sure it has to be destroyed, but this is not my area of expertise. I looked through an example of how to use QCompleter and did not see anything specific to destruction of the QCompleter (https://doc.qt.io/qt-5/qtwidgets-tools-completer-example.html).

[frmdstryr]

In the changeModel function it looks like they delete the whole completer.

void MainWindow::changeModel()
{
    delete completer;
    completer = new QCompleter(this); // Parent set here
    completer->setMaxVisibleItems(maxVisibleSpinBox->value());

    switch (modelCombo->currentIndex()) {
    default:
    case 0:
        { 
            QFileSystemModel *fsModel = new QFileSystemModel(completer); // Parent set here
            fsModel->setRootPath(QString());
            completer->setModel(fsModel);
            contentsLabel->setText(tr("Enter file path"));
        }
        break;

I vaguely remember reading somewhere that passing the parent automatically deletes when the parent widget is deleted. Perhaps it's only needed when updating the model.

[bburan]

How would delete completer be implemented via PyQt5? del completer?

On Thu, Mar 3, 2022 at 10:01 AM frmdstryr @.***> wrote:

In the changeModel function it looks like they delete the whole completer.

void MainWindow::changeModel() { delete completer; completer = new QCompleter(this); // Parent set here completer->setMaxVisibleItems(maxVisibleSpinBox->value());

switch (modelCombo->currentIndex()) {
default:
case 0:
    {
        QFileSystemModel *fsModel = new QFileSystemModel(completer); // Parent set here
        fsModel->setRootPath(QString());
        completer->setModel(fsModel);
        contentsLabel->setText(tr("Enter file path"));
    }
    break;

I vaguely remember reading somewhere that passing the parent automatically deletes when the parent widget is deleted. Perhaps it's only needed when updating the model.

— Reply to this email directly, view it on GitHub https://github.com/nucleic/enaml/pull/480#issuecomment-1058330761, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACPSQXJRA774F7MZ57O5BDU6D5BHANCNFSM5PLE4DTA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you commented.Message ID: @.***>

[MatthieuDartiailh]

I had assumed that Qt was handling most of those details. If it is not, it does indeed make sense to go with your current approach. I did not review Wednesday since you mentioned you wanted to try the Completer approach, but I will do my best to review it this weekend.

[MatthieuDartiailh]

Sorry for the delay. Finding time is a bit challenging at the moment. I managed to fix the CIs though so could you rebase so that at least this is green ?

[ghost]

@MatthieuDartiailh Sorry for the force-push. I was playing with the options in the GitHub GUI to auto-update, but that was a merge instead of a rebase. All green now! :100:

[MatthieuDartiailh]

I finally took the time to review it, and I agree it looks good and can be merged as soon as a change log entry is added and the new example is added to the docs.

However, I would like to give some background as to why I pondered this for a bit and why I believe a solution based on a completer declarative object would be better in the long run.

The one time I needed completion was to complete f-string like fields in Exopy. I ended needing this feature both for simple field and multi-line fields. I implemented both through RawWidget here https://github.com/Exopy/exopy/blob/main/exopy/utils/widgets/qt_completers.py

Compared to the proposed implementation, I needed to propose completion only at a specific time of typing which is the first thing that made me doubt the proposed approach. I also need some ways to refresh the entries but that is a detail.

However in the long existence of this code I never got to generalize it to add it to enaml so I don't want to hold back a useful implementation on the thin hope that I could come up with something nicer that would still allow to implement the proposed behavior so we would not have backward compatibility concerns.

[bburan]

@MatthieuDartiailh I agree with your thoughts. The choice to do an AutocompleteField as opposed to binding a completer declarative object was a matter of expediency and simplicity for me.

When I attempted to implement your preferred approach, I realized that it would be fairly complicated to get it working with a multi-line field. Hooking the completer into a field vs. multi-line appears to be different enough that the code path would be different depending on whether the completer parent was a single-line field vs multi-line field. We'd have to implement the following:

• A callback to determine whether the pop-up/autocomplete should be triggered
• How the autocomplete should appear
• A means for refreshing or querying (e.g., a URL endpoint) for autocomplete (i.e., this could be done as a callback)

At that point, I wonder if it's better to just create an example or recipe (e.g., based off of your RawWidget example in exopy).

In general, I view the AutocompleteField as supplementing the ObjectCombo. This is exactly why I wrote the AutocompleteField. We had a list of choices the user must select from, but the list was several hundred items long so the ObjectCombo became a bit unwieldy.

[MatthieuDartiailh]

I agree with your though but given my current bandwidth, I suggest we just merge this as is as soon as 0.15.1 is out with your fixes for Qt6. It will be part of 0.16.0 that I hope to release it before June and the beta freeze of Python 3.11 which will be a lot of fun to support.

If anybody wants to implement a more complete completer solution starting from they can start from your summary.

[codecov-commenter]

Codecov Report

Merging #480 (654779c) into main (e97409a) will decrease coverage by 0.08%. The diff coverage is 34.54%.

@@            Coverage Diff             @@
##             main     #480      +/-   ##
==========================================
- Coverage   73.16%   73.08%   -0.09%     
==========================================
  Files         317      319       +2     
  Lines       24125    24180      +55     
  Branches       55       55              
==========================================
+ Hits        17652    17671      +19     
- Misses       6473     6509      +36     

[bburan]

@MatthieuDartiailh I added the example to the docs and an entry to the changelog. This should be ready to merge whenever deemed appropriate. Thanks for your feedback!

[MatthieuDartiailh]

Thanks. There appears to be a conflict but we can fix it when merging.

[bburan]

Ok, I rebased off of main which should help with the merge conflict. I don't see the CI running after I rebased, though.

[MatthieuDartiailh]

You now have different conflicts with the CIs configuration in particular, not sure what you did.

[bburan]

Ok, I was rebasing against an old version of Enaml's main branch (buranconsult/enaml), which is why it got messed up. After rebasing against nucleic/enaml it worked. The merge conflict was in the changelog.

[bburan]

@MatthieuDartiailh I'm wondering if we should just close this a won't implement?

[MatthieuDartiailh]

Sorry I still plan to merge this at some point but between the bug fixes and work on bytecode to support 3.11 and given the limited time I have for OSS I am quite a bit behind.

[bburan]

No worries. I also have been a bit swamped myself with some new grants that have come in. I am hoping to get back to helping out with the Qt6 bugfixes in a few months once I get the new experiments up and running.