Open source rewrite of rqt_topic #47

flynneva · 2024-01-21T16:12:10Z

open sourced from Apex.AI

Closes #46

flynneva · 2024-01-21T16:13:36Z

fyi @methylDragon @wjwwood for some reason the tests are failing after migrating to rolling, so give me a few days to fix that and then we should be good to get this reviewed / merged in.

wjwwood · 2024-01-23T16:29:57Z

I'll have to defer to @methylDragon on this one, but feel free to ping me if you need my opinion on anything @methylDragon.

flynneva · 2024-01-29T17:57:48Z

@methylDragon @clalancette feel free to have a review of this PR. It's just a start on the rewrite for sure, but I think it gets this GUI in a much more usable state than before 👍🏼

Interested to hear your thoughts!

Also - happy to help maintain this repo if this PR is accepted.

flynneva · 2024-01-29T17:59:33Z

test/models/test_message_detail.py

+
+
+def test_message_detail_model(message_detail, qtmodeltester):
+    # qtmodeltester.check(message_detail)


I'll make a follow-up issue to tackle re-enabling these checks if this PR is merged in. I'm not quite sure why its failing all of a sudden once we open sourced this tool and dont have the time right now to dig into it.

clalancette · 2024-01-29T18:04:57Z

@methylDragon @clalancette feel free to have a review of this PR. It's just a start on the rewrite for sure, but I think it gets this GUI in a much more usable state than before 👍🏼

I have a few high-level comments here:

This is great! rqt_topic definitely needs some love.
There's a lot of changing of ' to " in this PR, which doesn't conform to our current style. This means this will almost certainly fail our flake8 tests, which need to pass for this to get in.
If we are going to heavily refactor this package, I think we should also refactor it to use more of our best practices. In particular, the layout of the code should conform much more closely to our recommended best practice. You can see that in any of the packages in e.g. https://github.com/ros2/ros2cli .
Finally, I'm not a huge fan of complete rewrites. They are difficult to review and hard to explain to users. I'd much rather see this as a series of improvements to the current code, with the goal of getting towards something like this PR. That will make it much more likely for us to get this in. If I were going to do this, then what I'd do is to first refactor the code as I mentioned in 3, then do a series of PRs where you introduce/refactor one part at a time.

Let me know if this makes sense to you.

flynneva · 2024-01-29T18:22:01Z

There's a lot of changing of ' to " in this PR, which doesn't conform to our current style. This means this will almost certainly fail our flake8 tests, which need to pass for this to get in.

@clalancette sure, this is an easy search and replace 👍🏼

If we are going to heavily refactor this package, I think we should also refactor it to use more of our best practices. In particular, the layout of the code should conform much more closely to our recommended best practice. You can see that in any of the packages in e.g. https://github.com/ros2/ros2cli .

@clalancette what "layout" in particular are you referring to in the linked repo? The linked repo contains many packages...but this repo only contains one. Are you suggesting to split up rqt_topic into many smaller packages? Or do you just mean removing the src directory and having rqt_topic at the top level?

Finally, I'm not a huge fan of complete rewrites. They are difficult to review and hard to explain to users. I'd much rather see this as a series of improvements to the current code, with the goal of getting towards something like this PR. That will make it much more likely for us to get this in. If I were going to do this, then what I'd do is to first refactor the code as I mentioned in 3, then do a series of PRs where you introduce/refactor one part at a time.

@clalancette sure, I agree as well. I just did it this way because internally the rewrite was already complete. If you'd like me to break this up into smaller chunks I can...just it will take much longer probably (months) to get this complete.

clalancette · 2024-01-29T18:50:32Z

@clalancette what "layout" in particular are you referring to in the linked repo? The linked repo contains many packages...but this repo only contains one. Are you suggesting to split up rqt_topic into many smaller packages? Or do you just mean removing the src directory and having rqt_topic at the top level?

The latter; removing src and having rqt_topic at the top level.

@clalancette sure, I agree as well. I just did it this way because internally the rewrite was already complete. If you'd like me to break this up into smaller chunks I can...just it will take much longer probably (months) to get this complete.

I think it is going to take months either way. But with smaller chunks we can actually see forward progress, which I think is a good thing.

flynneva · 2024-02-02T04:54:20Z

@clalancette took care of the src dir thing and migrating to use single quotes '. How would you like this PR split up? There is no real good way to "slowly" migrate without breaking things. Just let me know how you'd like to get this split up and I can do it 👍🏼

- open sourced from Apex.AI Signed-off-by: Evan Flynn <[email protected]>

Signed-off-by: Evan Flynn <[email protected]>

- meant to be ran in background threads instead of main GUI thread Signed-off-by: Evan Flynn <[email protected]>

- Views are split up into three main widgets - Topic list view that lists all available topics and their info - Message list view that lists messages from all selected topics - Message detail view that shows a detailed tree view of a single message Signed-off-by: Evan Flynn <[email protected]>

evan-flynn-apexai · 2024-03-25T19:09:40Z

But with smaller chunks we can actually see forward progress,

@clalancette I've broken up the single big commit into smaller more manageable ones for you to review. Let me know if you'd like me to split these out into their own MRs, though I dont think we can do this without breaking things.

P.s. sorry about my github accounts - I always forget which one I'm signed into. 😓

Signed-off-by: Evan Flynn <[email protected]>

flynneva requested a review from methylDragon as a code owner January 21, 2024 16:12

flynneva mentioned this pull request Jan 21, 2024

Open source rewrite of rqt_topic #46

Open

flynneva changed the title ~~Open source rewrite of rqt_topic from Apex.AI~~ Open source rewrite of rqt_topic Jan 22, 2024

flynneva force-pushed the rolling branch from c548a72 to b6898ae Compare January 29, 2024 17:56

flynneva commented Jan 29, 2024

View reviewed changes

flynneva force-pushed the rolling branch from b6898ae to 90fefcc Compare January 29, 2024 18:18

flynneva force-pushed the rolling branch 2 times, most recently from e617adb to 279dc1f Compare February 2, 2024 04:53

evan-flynn-apexai added 4 commits March 25, 2024 12:08

Open source rewrite of rqt_topic

69368e0

- open sourced from Apex.AI Signed-off-by: Evan Flynn <[email protected]>

Add new folder to hold common button QAction objects

eb038d0

Signed-off-by: Evan Flynn <[email protected]>

Add background worker objects

de8c4b6

- meant to be ran in background threads instead of main GUI thread Signed-off-by: Evan Flynn <[email protected]>

flynneva force-pushed the rolling branch from 5d8d752 to d146e40 Compare March 25, 2024 19:08

flynneva force-pushed the rolling branch 3 times, most recently from 2e58b38 to 6d67d10 Compare March 25, 2024 19:35

evan-flynn-apexai added 2 commits March 25, 2024 12:40

Add new rqt_topic models, with tests

96a51b8

Signed-off-by: Evan Flynn <[email protected]>

Bring rqt_topic rewrite together, use in main

3ed3ed7

Signed-off-by: Evan Flynn <[email protected]>

flynneva force-pushed the rolling branch from 6d67d10 to 3ed3ed7 Compare March 25, 2024 19:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Open source rewrite of rqt_topic #47

Open source rewrite of rqt_topic #47

flynneva commented Jan 21, 2024 •

edited

Loading

flynneva commented Jan 21, 2024

wjwwood commented Jan 23, 2024

flynneva commented Jan 29, 2024

flynneva Jan 29, 2024 •

edited

Loading

clalancette commented Jan 29, 2024

flynneva commented Jan 29, 2024 •

edited

Loading

clalancette commented Jan 29, 2024

flynneva commented Feb 2, 2024

evan-flynn-apexai commented Mar 25, 2024 •

edited

Loading



		def test_message_detail_model(message_detail, qtmodeltester):
		# qtmodeltester.check(message_detail)

Open source rewrite of rqt_topic #47

Are you sure you want to change the base?

Open source rewrite of rqt_topic #47

Conversation

flynneva commented Jan 21, 2024 • edited Loading

flynneva commented Jan 21, 2024

wjwwood commented Jan 23, 2024

flynneva commented Jan 29, 2024

flynneva Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

clalancette commented Jan 29, 2024

flynneva commented Jan 29, 2024 • edited Loading

clalancette commented Jan 29, 2024

flynneva commented Feb 2, 2024

evan-flynn-apexai commented Mar 25, 2024 • edited Loading

flynneva commented Jan 21, 2024 •

edited

Loading

flynneva Jan 29, 2024 •

edited

Loading

flynneva commented Jan 29, 2024 •

edited

Loading

evan-flynn-apexai commented Mar 25, 2024 •

edited

Loading