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.

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

UI for metadata filters #484

Closed
wants to merge 28 commits into from
Closed

UI for metadata filters #484

wants to merge 28 commits into from

Conversation

pierrotsmnrd
Copy link
Contributor

This PR adds a UI to open a new chat, either by uploading a document, or by using the existing corpus and adding metadata filters.

Enregistrement.de.l.ecran.2024-08-06.a.17.55.51.mov

@pierrotsmnrd pierrotsmnrd requested a review from pmeier August 8, 2024 08:26
Comment on lines +10 to +11
"size": {"type": int},
"extension": {"type": str},
Copy link
Member

Choose a reason for hiding this comment

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

We can keep this for now as it is supplied by our default LocalDocument, but they have to be variable before we can merge into main.

Comment on lines 211 to 213
return self.convert_operator(self.operator)(
key=self.key, value=self.convert_value(self.value)
)
Copy link
Member

Choose a reason for hiding this comment

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

Instead of letting convert_operator return the convenience method, let's have it return the actual operator and instantiate the MetadataFilter manually:

Suggested change
return self.convert_operator(self.operator)(
key=self.key, value=self.convert_value(self.value)
)
return MetadataFilter(
operator=self.convert_operator(self.operator),
key=self.key,
value=self.convert_value(self.value),
)


self.delete_buttons = []

# dummy_row = FilterRow(key="document_name", operator="==", value="applications.md")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# dummy_row = FilterRow(key="document_name", operator="==", value="applications.md")

self.metadata_filters = self.metadata_filters + [new_metadata_filter_row]

def delete_metadata_filter(self, event):
print("should delete filter : ", event)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
print("should delete filter : ", event)

Comment on lines +291 to +293
if not filter.validate():
result = False
# Do not break, we want to call validate() on every filters
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's take this example :
Capture d’écran 2024-08-14 à 09 44 14

We have 2 empty fields. We want to validate all of them, and not stop after the first, to highlight in red all the invalid ones :
Capture d’écran 2024-08-14 à 09 44 41

Copy link
Member

Choose a reason for hiding this comment

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

In that case the variable naming is confusing. So filter is not a row, but rather one element inside the row? Maybe name it filter_component than?

Comment on lines 150 to 154
asyncio.ensure_future(
self.did_finish_upload(
self.metadata_filters_builder.get_metadata_filters()
)
)
Copy link
Member

Choose a reason for hiding this comment

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

Why not make this method async and just await the result?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The function is a button callback. I have had issues in the past with making such callbacks async. But that seems to work now, so let's do it.

@pmeier pmeier mentioned this pull request Aug 21, 2024
@pmeier
Copy link
Member

pmeier commented Sep 5, 2024

Superseded by #499.

@pmeier pmeier closed this Sep 5, 2024
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