Skip to content

Conversation

xiangzhi
Copy link
Contributor

@xiangzhi xiangzhi commented Jun 11, 2021

Implementation of the feature discussed in #157

The main changes are as follows:

  1. Dataset.cs know takes in an optional save path and autosave variable. If true, any dataset structural changes are automatically saved upon the completion. This also applies to Sessions where if new derived partitions are created, the dataset is saved as well.
  2. Tests for the changes above.
  3. PsiStudio now has an extra setting AutoSaveDataset that when set to true, all dataset created/loadeed will be in autosave mode IF a save path is known.

I also created a property called UnsavedChanges that is toggled if autosave is disabled. This would let us check if any changes has been done since the last save. I haven't figure out how to hook it up with the name in PsiStudio.

Any feedback and suggestion on better implementation is welcome!

@danbohus danbohus self-requested a review June 11, 2021 19:39
Copy link
Contributor

@danbohus danbohus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, thanks a lot for this PR! I made a few comments, most of them stylistical in nature, let me know what you think.

@xiangzhi
Copy link
Contributor Author

Thanks Dan! I just made all the suggested changes. I added the events DatasetChanged and SessionChanged that are called when the structure of either are changed. I'm not familiar with the correct design, so let me know if there is a better way to code events.

Besides that, I also propagated the change of Save As all the way into PsiStudio and it says Save As Dataset now instead of Save Dataset. This should make the future implementation of Save easier.

Copy link
Contributor

@danbohus danbohus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for the changes! I suggested a couple of small additional changes. It would be great to call the events a minimum number of times, at the very bottom of the API stack and have the higher level API calls not duplicate the event handler call.

@xiangzhi
Copy link
Contributor Author

xiangzhi commented Jun 17, 2021

I just made the suggested changes. It should be ready to go!

I did found a few more places where the OnDatasetChanged got called multiple times. For example:

public Session AddSessionFromStore(IStreamReader streamReader, string sessionName = null, string partitionName = null)
{
    var session = new Session(this, sessionName ?? streamReader.Name);
    session.AddStorePartition(streamReader, partitionName);
    this.AddSession(session);
    return session;
}

Both session.AddStorePartition (it calls AddPartition later) and this.AddSession invokes the event at different level. Since the API can be called independently, we cannot simply remove it. We could have variables to indicate its a nested function or a bunch of private methods. Either way would involve tinkering with large part of the Session and Dataset. I think its an acceptable trade-off for now, but someone with a huge JSON file might run into an issue in the future😅.

Copy link
Contributor

@chitsaw chitsaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @xiangzhi!

@danbohus danbohus merged commit 9b0c6a2 into microsoft:master Jun 22, 2021
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.

[Feature Request] PsiStudio prompt save when exiting if any changes to dataset are made.

3 participants