Additional command-line params and support for plugin notifications

[GerardoPardo]

Added command-line parameters that configure various including window titles, content of the "about' window, the splash image, set of enabled and disabled plugins, selecting a "default" streamer, auto starting the "default" streamer.

Added support for managing streaming plugin notifications

Added support for streaming plugin start/stop "out of band" so that PJ can always know whether the plugin is running or not.

[facontidavide]

Hi Gerardo.

I think that this PR is too big for its own sake. This makes it more complicated for me to review it.

My initial suggestions are listed in the pending review.

Additionally:

1. Why read strings from file (MainWindow::ReadFileContent) instead of passing directly a string?
2. I need to review in detail the "notification mechanism"

[GerardoPardo]

Hi, Thanks for the comments. See below:

On Sun, Aug 15, 2021 at 9:10 AM Davide Faconti ***@***.***> wrote: Hi Gerardo. I think that this PR is too big for its own sake. This makes it more complicated for me to review it. My initial suggestions are listed in the pending review.

I do not see this. Do you have a link to these suggestions or are they still not visible on github?

Additionally: 1. Why read strings from file (MainWindow::ReadFileContent) instead of passing directly a string?

These are the ones that read the MainWindow::_about_title and MainWindow::_about_body, correct? Perhaps the name of these variables and command-line parameters is misleading. They are HTML-formatted text and it is a bit involved so it is not so easy to pass directly in the command line and it might require escaping some chars. You can see the contents of the files I am using under: plotjuggler_app/resources/html/about_window_title_rti.html plotjuggler_app/resources/html/about_window_body_rti.html Which brings up another thing. Perhaps you think it is not appropriate to have these files in the main PLotJuggler directory but I did not see a simple way to have them "compiled in" in the resource.qrc file... By the way if you build the cnxt_wl branch in https://github.com/rticommunity/PlotJuggler/tree/cnxt_wl you can see the impact of all these parameters in the OOB. To see this run: plotjuggler --subscribe true --title "PlotJuggler - RTI Connext DDS Edition" --splash ://resources/pj_splash_rti.jpg --about_title ://resources/html/about_window_title_rti.html --about_body ://resources/html/about_window_body_rti.html --enabled_plugins "libDataLoadCSV;libDataLoadULog;libDataStreamRTI;libDataStreamSample;libPublisherCSV" --selected_streamer "RTI Connext DDS" My intent was build a version of PlotJuggler where I add the following to the plotjuggler_app/CMakeLists.txt add_definitions(-DPJ_DEFAULT_ARGS="--subscribe true --title PlotJuggler_0x20_-_0x20_RTI_0x20_Connext_0x20_DDS_0x20_Edition --splash ://resources/pj_splash_rti.jpg --about_title ://resources/html/about_window_title_rti.html --about_body ://resources/html/about_window_body_rti.html --enabled_plugins libDataLoadCSV_0x3b_libDataLoadULog_0x3b_libDataStreamRTI_0x3b_libDataStreamSample_0x3b_libPublisherCSV --selected_streamer RTI_0x20_Connext_0x20_DDS") Note the "_0x20_" and "_0x3b" which are used to escape the whitespace and ';' chars. This is handled by MergedArguments() in main.cpp With this in place I can run plotjuggler with no command-line parameters and it gets all those parameters automatically. They can still be overridden if you pass them explicitly. As mentioned this may not be the best way to get the end result. The goals I cared about where: (1) the the "automatic" OOB experience the you can see when running with the options I listed above (2) Ideally have a way to get this experience without having to pass command-line paramaters as that would require always running through some sort of launching scrip. This is not a must have since likely we will have this script in order to locate the Connext Core Libraries but I thought it was a nice possibility since it only requires building the program with an extra macro defined (PJ_DEFAULT_ARGS). (3) Have a minimal deviation from the main PJ codebase so we do not need to continually merge changes done to the main repo. I was targeting "no deviations" by having these parameters added to PJ as they are not Connext specific... If you have alternative/better suggestions on how to accomplish this it might be good to make this changes before merging the pull request...

1. I need to review in detail the "notification mechanism"

OK. This is indeed the more involved change. Gerardo

…

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#489 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABA3YLH5RIIQMUTYNNTZXQ3T47RHLANCNFSM5CEB6HPQ> . 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&utm_campaign=notification-email> .

[facontidavide]

ok, Let me cherry-pick some of your changes and do it in "my way".

Since once this is merged, I become responsible to support this, I need to be sure that these changes are consistent with the vision I have 😄

[facontidavide]

In the last commit f7959a4 I added some of the features you suggest with a twist:

• disabled_plugins and enabled_plugins work, in the same way, but I added some additional qDebug messages.
• Instead of the commands title, splash, about_title and about_body, there is a single one called "skin_path" where all the files can be found. Have a look at a sample here: https://github.com/facontidavide/PlotJuggler/tree/main/plotjuggler_app/resources/skin

[facontidavide]

More changes pulled from your PR.

8be5c05

The outstanding modifications you suggest are:

• Automatically start a plugin (I will think take care of this feature miself, don't worry)
• Add signal DataStreamer::runStatusChanged . I think this is redundant in scope with DataStreamer::closed, am I missing something? It is OK for a plugin to stop itself (generally after an error), but no to start itself, in my opinion.

I suggest closing this PR and if you want to discuss the other features, we can do it in smaller and more focused Pull Requests.

[facontidavide]

By the way, I modified the dummy streamer (launch with option -t). You can see there how my modified notifications work (I will keep the bell always visible but disabler or enabled).

[GerardoPardo]

Closing the PR sounds reasonable, I will pull and merge your changes and if I see something missing we can always create a different, smaller PR. Regarding the signal DataStreamer::runStatusChanged . The issue is that we added the ability to "Start/Stop" the DDS Participant from the configuration dialog. Since the configuration dialog can be opened directly from the "options" menu (the gear icon) the main window I did not see any other way to notify the main window that the plugin had been started. In the case of te DDS plugin I think it is natural for the plugin to start itself, not just to stop itself. Otherwise we would need to split the 'configuration' window into one that dela only with start/stop and another with the rest of the configuration. I think this would complicate things unnecessarily. Do you have some other suggestion on how to handle this?

…

On Mon, Aug 30, 2021 at 2:53 AM Davide Faconti ***@***.***> wrote: More changes pulled from your PR. 8be5c05 <8be5c05> The outstanding modifications you suggest are: - Automatically start a plugin (I will think take care of this feature miself, don't worry) - Add signal DataStreamer::runStatusChanged . I think this is redundant in scope with DataStreamer::closed, am I missing something? It is OK for a plugin to stop itself (generally after an error), but no to start itself, in my opinion. I suggest closing this PR and if you want to discuss the other features, we can do it in smaller and more focused Pull Requests. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#489 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABA3YLD76VG4CUV6MOHH5JDT7NIJBANCNFSM5CEB6HPQ> . 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>.

[facontidavide]

I added the option "start_streamer" 1a716ba

To quickly test it:

         plotjuggler -t --start_streamer libDataStreamSample