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

lcm-logplayer-gui: No .jlp with only defaults #530

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

judfs
Copy link
Contributor

@judfs judfs commented Oct 16, 2024

If you just open and close a log, without clicking on any knobs, don't create a .jlp file.

google-java-format was suggested at #420 (comment) . I made a commit using it on the file I was working with first. Looking at the diff of 36a670e should be sane.

@judfs
Copy link
Contributor Author

judfs commented Oct 16, 2024

Once again I dared to use unicode. -encoding utf8 made the tests pass at least.

@nosracd
Copy link
Contributor

nosracd commented Oct 23, 2024

I like intent of this PR. It seems a lot more sane to skip writing out a .jlp if the user does nothing but click the GUI window to close it.

One of the things I liked about the old behavior was this:

  • Open a long log file in GUI (some channels show up, but not all)
  • Play through log file. By the end, all of the channels in the log are showing up in the GUI
  • Close GUI
  • Reopen log file in GUI (now all channels in the log show up in the GUI)

But in this PR, a .jlp isn't saved unless I disable a channel. What do you think about preserving the above behavior?

@judfs
Copy link
Contributor Author

judfs commented Oct 24, 2024

My motivation for this change is that it is inconvenient for additional files to be created when just playing back a log and doing nothing fancy. My team shares datasets as folders of LCM logs and random jlp files are undesired clutter. To this end, I would like not creating a jlp file when simply playing a log to be the default. "But in this PR, a .jlp isn't saved unless I disable a channel. What do you think about preserving the above behavior?": That was my entire goal.

I'd be interested in a opt in "Always make jlp" user preference.

I'd also be interested in a button to create a more proper index file for a log. I can imagine recording each of the following per channel:

  • firstEventTime
  • firstEventSeek
  • numEvents
  • lastEventTime
  • totalBytes?
  • ???

logplayer-gui could then populate the filterlist from an index.

@kyonifer
Copy link
Contributor

@judfs what are your thoughts about keeping the original behavior as the default, but adding a flag that enables the behavior you suggested above?

My preference is to keep the original behavior as the default to provide UX stability for users who have come to expect that behavior, but I definitely agree that there are use cases where the default is inconvenient. We could also consider environment variables instead of command line flags.

@judfs
Copy link
Contributor Author

judfs commented Oct 24, 2024

How about a dialogue on close?

Save log-player-gui session (.jlp)?
[Yes] [No] Remember this for future logs [ ]

@nosracd
Copy link
Contributor

nosracd commented Nov 1, 2024

In a side-channel, we discussed this a bit more. In all, the solutions we've considered are:

  1. Keep the existing behavior for continuity's sake. Add a new opt-in option to disable the existing behavior to improve the UX.
  2. Change the existing behavior to only write a .jlp when the user turns off a channel. Add a new opt-in option to preserve the previous behavior.
  3. Change the existing behavior to never write a .jlp. Add a new opt-in option to preserve the previous behavior.
  4. Add support for a .nojlp sentinel/marker file. If such a file exists alongside the log then the logplayer-gui will not write a .jlp.
  5. Move where the .jlp files are stored from beside the log file to somewhere in the user/system settings.
    1. We could use the filepath to the log to match the log setting to the file. Although this means if you move the log file you will lose your settings.
    2. We could hash the log file to match the settings to the log. This will introduce performance issues. It also means that there can no longer be multiple directories containing the log with different settings.

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