Stable version of notification filter#34
Stable version of notification filter#34Daniel-Meza wants to merge 8 commits intoPaperFanz:noeticfrom
Conversation
AdamPettinger
left a comment
There was a problem hiding this comment.
Great work @Daniel-Meza! This looks very good. I mostly left cleanup and cosmetic recommendations/comments.
I think the biggest 2 changes I would want to see are:
- Move the
Notification-specific padding at the topic of the multiline print to the Notification filter itself so that printing function becomes more agnostic. Left a comment about this in the code - Didn't leave a comment about this, but I dislike the order the queue is displayed. New messages are added to the bottom so that the oldest messages are on top. That seems backwards of what it should be to me. Maybe @PaperFanz has input about this. It would probably be pretty easy to have the new messages appear on the top: switching from
queuetodequeis probably the way
|
@AdamPettinger First, I've yet to change the order of the queue. My reasoning was to make it have the order of a terminal, where new messages are at the bottom. But I'm not set on either so just let me know and I can put new messages on top. Secondly, there are still those debugging lines commented out. Need them to finalize the change above so those will be removed next time. |
AdamPettinger
left a comment
There was a problem hiding this comment.
Look good @Daniel-Meza!
|
All comments above have been addressed. Let me know if you encounter any problems. |
AdamPettinger
left a comment
There was a problem hiding this comment.
There was one change needed to get this to build after the recent batch of changes.
While testing this more thoroughly I discovered a problem: the placement of the drawn text is weird depending on the length of the topic. Try this:
- Change topic name to "test" -> text should disappear
- Change topic name to "reallyreallyreallyreallyreallylongstring" -> text is way lower in the box
- Change topic name to "elevenchars" -> text is half off the top side of the box
I don't really know what is causing this, but for topic names of less than 11 characters, the filter text disappears. Long long topic names puts it very low on the screen. Thoughts?
PaperFanz
left a comment
There was a problem hiding this comment.
The code looks good at a glance but make sure you are running the format script before committing every time. With these types of pull requests it may be best to include screenshots or even a short screen capture to demonstrate the functionality of the pull request.
|
Made changes to the filter. |
… addtions + bug fixes
Co-authored-by: AdamPettinger <[email protected]>
…vents text from disappearing from the textbox as its size or the number of lines to display change
f398ad2 to
878f121
Compare
AdamPettinger
left a comment
There was a problem hiding this comment.
I just tested the most recent changes, and everything looks really good!
Great job @Daniel-Meza!
First working version of notification filter.
The filter works as follows:
Message Type: std_msgs/String
Format: "data: 'This is a message'"