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

Add automatic data reload on change #667

Closed
wants to merge 2 commits into from

Conversation

agoessling
Copy link
Contributor

This adds a feature which automatically reloads log data based on the last modified date. This allows new data (e.g. from a simulation run) to be displayed without user intervention simply by overwriting the log file.

NOTE: This feature is currently enabled by default but in the end should probably be enabled via a preference either globally or in the data load dialog. I'll wait for suggestions on the most suitable location and best way to accomplish this.

Discussion: #653

Previously the CSV dataload plugin was not saving the correct XML state
when a generated time axis was used.
This adds a feature which automatically reloads log data based on the
last modified date.  This allows new data (e.g. from a simulation run)
to be displayed without user intervention simply by overwriting the log
file.
@Bartimaeus-
Copy link
Contributor

I like this feature, am wondering about the behavior when a csv file is renamed/moved/deleted externally
Example handling (from Notepad++):
Query if user wants to keep the file loaded
image
If you choose to keep the file loaded Notepad++ will continue to monitor for the original file and ask if you want to reload it if becomes present again:
image

@agoessling
Copy link
Contributor Author

Right now this feature has the same behavior as what you describe in Notepad++ except that it doesn't prompt the user when the file is no longer present, but does continue to monitor it in case it becomes present. What is the use case where you would want the data to be removed from Plotjuggler when the log file goes away?

@agoessling
Copy link
Contributor Author

@Bartimaeus- I think having a dialog might defeat the purpose of seamlessly automatically reloading. Is there a use case I'm not considering?

@facontidavide Any thoughts on how / where to integrate the bool flag to enable this feature?

@facontidavide
Copy link
Owner

Hi,
thanks for taking the time to address this, but I am afraid that I can not accept this PR.

This might work for your particular load plugin, but it is undefined behavior for any other plugin.

A solution to solve your problem would be to use a streaming plugin instead, but then there would be a LOT of code duplication related to CSV parsing.

Summarizing, this is not a change I want to include.

@agoessling
Copy link
Contributor Author

@facontidavide Thanks for reviewing.

Why is this undefined behavior for other plugins? My understanding was that the FileLoadInfo struct provided all the information necessary to reload the datafile as is done while loading a saved layout XML?

If this is indeed not possible with other plugins, could we make this feature CSV specific? For example, add an Enum to FileLoadInfo that describes the plugin that was used for this dataset and use this to modify behavior in CheckLogs?

@facontidavide
Copy link
Owner

The fact that we CAN do something doesn't mean we should.
I am sorry but I think that calling the loadDataFromFile funtion periodically is wrong and must not be done.

@agoessling
Copy link
Contributor Author

@facontidavide I agree that loadDataFromFile should not be called periodically. In fact, it is only called if the underlying data has been modified. Which would be never in most cases. I also think there should be a flag somewhere in the preferences to enable this feature and was looking for guidance on where you would suggest putting it.

I understand if you simply don't want to include the feature, but for my education / understanding I'm trying to understand if your objection is on technical grounds or simply product vision.

@facontidavide
Copy link
Owner

facontidavide commented Jun 28, 2022

Product vision, mostly.

@agoessling
Copy link
Contributor Author

@facontidavide Fair enough. If you can think of a way that this type of behavior / feature would better fit into your vision I'd be happy to explore / implement it. It's a pretty big win for our organization's use case. Also if you have technical concerns you'd be willing to share I'd love to hear them just to learn.

@facontidavide
Copy link
Owner

facontidavide commented Jun 28, 2022

is the data appended to the CSV or is the entire csv modified?
If data is just appended, the "good" solution is to convert this into a streaming plugin

@facontidavide
Copy link
Owner

It's a pretty big win for our organization's use case.
Organizations that want to influence the development of PlotJuggler should conside sponsor it 😉

@agoessling
Copy link
Contributor Author

In our case, the CSV is overwritten with new data, although I could see the utility for a CSV streaming plugin for other applications.

Organizations that want to influence the development of PlotJuggler should conside sponsor it 😉

Is sponsorship the preferred mechanism for supporting open source projects over submitting PRs for bug fixes / features? I wasn't trying to influence so much as a provide a PR for a new feature. Admittedly I'm not very familiar with SponsorWare.

@facontidavide
Copy link
Owner

Don't take me wrong. All the PR are greatly appreciated and welcome, and I take my time to look at them.
But I merge only those that are aligned with my vision of the software and don't add complexity.

This particular behavior, in my opinion, breaks the implicit rules of how a DataLoad plugin should work.

@agoessling
Copy link
Contributor Author

This particular behavior, in my opinion, breaks the implicit rules of how a DataLoad plugin should work.

I believe this PR doesn't change how DataLaod plugins work. It does change when they are invoked. Namely, only when a previously loaded file is modified. Anyway, if you can think of a way a feature similar to this would not break your implicit rules, let me know and I'll try to submit something.

@agoessling agoessling deleted the data_reload branch August 11, 2023 22:17
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