Dijji / XstReader

Xst Reader is an open source viewer for Microsoft Outlook’s .ost and .pst files, written entirely in C#. To download an executable of the current version, go to the releases tab.
Microsoft Public License
479 stars 70 forks source link

UpdateView functions in View and FolderView #42

Closed iluvadev closed 3 years ago

iluvadev commented 3 years ago

I think all runs ok, at least in the manual tests I have done. I made some minor changes: XstFile:

FolderView:

View:

MainWindow:

Dijji commented 3 years ago

These changes are mostly all to the good. I don't agree with adding the RootFolder property, though, as I think the core's caller should be in control of the lifetime of the data read from the Outlook file, and adding this property holds it in memory in a way that is both opaque and unavoidable. I do agree with setting the Folder property in Message where you do, but I think I would have used a .ForEach clause in the Linq statement to avoid an additional loop construct.

Also, there is no need to leave old code in place commented out, the differencer tells me what has changed quite clearly. It is better just to change the code to what you think it should be, so that it is ready for check-in, and leave any explanation in the pull request's comments.

What do you think we should do next with these changes?

I am currently working on testing the core API by adding a second client in the form of a command line export tool, starting from the baseline provided by another contributor.

Dijji commented 3 years ago

The updated changes all look good to me. There are a bunch of definite improvements. I am curious, though, why you introduce the Internal restriction on things like NIDs?

iluvadev commented 3 years ago

Hey, sorry, I haven't written it before ... I thought you wouldn't see it if I didn't make a new pull-request ... I'm new to git

I think the properties and methods should be visible only where they should be used. All the properties were public, but I think that some are just for internal structures and data. I think this information is only for the Core. UI works with processed and structured data by the Core. Maybe is not important... In the case of defining a public API, it is important to have good control over everything that can be “seen” outside, to avoid mistakes and misuses.

iluvadev commented 3 years ago

The main changes I have made are:

I have tested and I think everything works fine.

Dijji commented 3 years ago

I like it! Accepting the pull. Thank you very much.

Dijji commented 3 years ago

FYI: I am pushing this towards release - the Divided branch contains the latest candidate, with a few more changes:

iluvadev commented 3 years ago

Perferct! I'm playing with the code in my fork to understand it. I'm trying to do some more improvements. I'm enjoying with your project 😄

Dijji commented 3 years ago

I have now shipped all these changes as Release 1.14, so everything is now back in master.

Thanks again for your contribution. It is much appreciated.