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

Topic filter #144

Closed
wants to merge 1 commit into from
Closed

Topic filter #144

wants to merge 1 commit into from

Conversation

EdJoPaTo
Copy link
Owner

The initial implementation just does topic.contains(filter) which is very simple. Is that enough? I thought about mqtt topic matching (+/status/#) or regular expressions (\/status\/) but that would be more complex quickly.

#103 thought about searching in the current payload too. Not sure how well that works performance wise. Also it might be not as easy to understand what's going on in such a case. Keeping it simple might be the best overall solution?

Other thoughts?

(You can test around with the build including the current filter mechanic prebuilt from here when logged in to GitHub: https://github.com/EdJoPaTo/mqttui/actions/workflows/rust.yml?query=branch%3Atopic-filter)

@EdJoPaTo
Copy link
Owner Author

Something that came up while talking about this feature: htop F3 Search vs F4 Filter.
Personally I prefer F4 but I understand that use cases can be different. I think still having all the topics and only search in them (F3) can also be useful…

@b-reich
Copy link
Contributor

b-reich commented Nov 12, 2023

I think the filter/search should be case insensitive. "Simple" pattern matching would be nice I think.

@b-reich
Copy link
Contributor

b-reich commented Nov 12, 2023

When clearing retained messages, it's essential to adhere to the specified filter criteria. For instance, consider the following topics:

home/livingroom/temp
home/bathroom/temp

Applying a filter for living and initiating a cleanup operation under the home topic should exclusively target and clean the relevant topic.

@devprofile98
Copy link

The current Simple functionality is working and solves lots of issues.

  • I think your thoughts on having topic matching is a better feature to have, mqttui can use rumqttc matches function to simply add the feature, I think.
  • using a feature-rich text editor should be a better choice, tui-textarea did it well.
  • also not being able to navigate between search results before pressing Enter is annoying, adding this functionality is useful to the end user.

If they make sense to you, I would be glad to add these features.

@EdJoPaTo
Copy link
Owner Author

EdJoPaTo commented Nov 13, 2023

thank you for your input! I have some thoughts on them. Guess there is no perfect way of doing it and I want to balance usefulness and simplicity (for both using and developing) with it.

  • I think your thoughts on having topic matching is a better feature to have, mqttui can use rumqttc matches function to simply add the feature, I think.

The downside of the MQTT topic matching is the required stuff like +/status/# to find all topics below status. Just typing status isn’t enough. Also it doesn’t provide partial text support. I will never find temperature when searching for temp. That’s why I think it’s probably to complicated for quick and simple usage.

  • using a feature-rich text editor should be a better choice, tui-textarea did it well.

Not sure if that’s over the top already. I like the simplistic approach of htop there: it’s only in the bottom line not obstructing the view of something else. Also the widget looks like it has way too much. There is no need for things like multiline or things like that. What’s currently missing what’s really needed?

  • also not being able to navigate between search results before pressing Enter is annoying, adding this functionality is useful to the end user.

That’s more like the F3 Search of htop while currently it’s more implemented like F4 Filter. Still not sure what’s better or more useful.

@EdJoPaTo
Copy link
Owner Author

Applying a filter […] and initiating a cleanup operation […] should exclusively target and clean the relevant topic.

Are there other things here being similar? I thought about just denying to use that feature while a filter is used as it's more complicated then.

Another thought is to not add the filter and only add a search. Then nothing like that is possible to confuse users in the first place.
The subscription topic is also possible on startup which is also way better performance wise (but has its downtimes too).

@b-reich
Copy link
Contributor

b-reich commented Nov 13, 2023

Applying a filter […] and initiating a cleanup operation […] should exclusively target and clean the relevant topic.

Are there other things here being similar? I thought about just denying to use that feature while a filter is used as it's more complicated then.

I don't like the idea of blocking the feature in general. Maybe a more explicit warning?

@devprofile98
Copy link

The downside of the MQTT topic matching is the required stuff like +/status/# to find all topics below status. Just typing status isn’t enough. Also it doesn’t provide partial text support. I will never find temperature when searching for temp. That’s why I think it’s probably to complicated for quick and simple usage.

I agree with you, I didn't feel that I need this feature in the past, but a workaround for this could be make a decision to use matches function when we have '$,' '*,' '+,' and '#' included, otherwise do the simple filtering.

Not sure if that’s over the top already. I like the simplistic approach of htop there: it’s only in the bottom line not obstructing the view of something else. Also the widget looks like it has way too much. There is no need for things like multiline or things like that. What’s currently missing what’s really needed?

No, not really.

That’s more like the F3 Search of htop while currently it’s more implemented like F4 Filter. Still not sure what’s better or more useful.

As a user, It felt bad not to be able to navigate between the results while searching. it is my opinion.

@EdJoPaTo
Copy link
Owner Author

it is my opinion.

I like that we are thinking about these things!

The downside of the MQTT topic matching is the required stuff like +/status/# to find all topics below status. Just typing status isn’t enough. Also it doesn’t provide partial text support. I will never find temperature when searching for temp. That’s why I think it’s probably to complicated for quick and simple usage.

I agree with you, I didn't feel that I need this feature in the past, but a workaround for this could be make a decision to use matches function when we have '$,' '*,' '+,' and '#' included, otherwise do the simple filtering.

I think I'd go with the simple solution of not including something like that at first. Keeping it simple.
When the need seems bigger its still something that can be added.


Somehow I think a search function would be more beneficial than a filter function. It has less downsides (the remove retained feature for example) and serves the purpose of finding topics of interest.

@EdJoPaTo
Copy link
Owner Author

For everyone looking into this: I created a topic search PR #145 which will likely supersede this PR. Feel free to join into the discussion and provide feedback! It was very helpful here to come to that point in the first place.

@EdJoPaTo
Copy link
Owner Author

Closing in favor of #145

@EdJoPaTo EdJoPaTo closed this Jan 15, 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