AboutDialog Lab

[afrlan]

This is initial implementation of AboutBox:

It retrieves info from Assembly, GitHub and VSMarketplace. It has basic parsing of Changelog.md file. (Sufficient to present all changes currently present; will be expanded in future iterations.)

[afrlan]

I closed it because I thought that newest commit isn't included. I was wrong.

[ironcev]

Hi @afrlan, thanks a lot for this pull request! I can hardly wait to have the dialog released :-)

The requirements are fulfilled very precisely I must say (up to one single detail ;-)). Also, the code is written in a very clean and understandable way.

My remarks are given below. As you will see, I would like to use the About Dialog to set the bar and guidelines for all the upcoming dialogs and UI elements in Sharpen. (And there will be plenty of them.)

Since this is a PR for the Lab and not for the final dialog there are just a few minor things to improve to fulfill the Lab requirements. Also, I would suggest implementing the missing functionality in the Lab project. So, to get the PR merged only the first two group should be implemented:

• Fulfill Lab Requirements
• Implement Missing Functionality

However, since you will soon integrate the dialog into the Sharpen project I will use this opportunity to give some hints related to the code so that it complies with (at the moment unwritten) Sharpen coding guidelines. Some architectural points are there as well. Fill free to implement these steps in a separate PR or in this one, as per your convenience.

Fulfill Lab Requirements

• [x] Name the folder AboutDialog instead of AboutBoxTest. Labs do not end in Test and Dialog and not Box is used in the issue definition.
• [x] Delete the copy of Sharpen's README.md. It was used for experimenting in MainWindow.xaml.cs but that code is commented out (and should also be deleted - see below).
• [x] Add Lab README.md. Each Lab project must have a readme that explains its purpose, usage, and outcome. As a reference take a look at READMEs in other Sharpen Lab projects. This is also a good example of a Lab README. The README.md has to be in the lab root folder (lab/AboutDialog). The README.md file is always in the root folder of a particular lab. In general a lab can contain several projects or subfolders and the README.md describes the whole lab and not only a single subfolder.

Implement Missing Functionality

• [ ] Handle values coming from GitHub and VSMarketplace in case of error.

According to the requirements:

If not or if any error happens during fetching do not display anything.

There will surely be issues when fetching data from GitHub and VSMarketplace. The current implementation is optimistic and assumes that the data will always be there. Implement a robust solution in case of errors. Per spec in that case e.g. number of stars etc. will simply not be displayed.

How to detect and handle the error case?

The newest implementation catches exceptions and ignores any further procession if an exception happens. We can get exceptions in case of e.g. network failure. But what will happen when fetching data over REST or GraphQL returns a non-200 HTTP response. E.g. 500 or similar? Will the used frameworks in that case internally convert a non-200 HTTP response to an exception. I did not work much with the two frameworks of choice but I do not expect them to do so. I expect that the used frameworks will most likely not throw any exceptions, but simply return error codes and JSON structures that will contain detailed error descriptions. It could be that the processing of those returned payloads in our code like e.g. JSON deserialization will throw exceptions, but are we sure that will happened?

To come to a bullet-proof solution we have to move away from any "shoulds" and know exactly how the used frameworks work. In plain words, I would explicitly check for a success response from the services and leave the exception handling for exceptions like e.g. network issues.

• [x] Handle the async void WindowLoaded in an optimized way. Since the implementation of the event handler is async we have a fire-and-forget case. This is exactly what we want because the opening of the dialog will not wait for the services to fetch data or for the changelog to be parsed. The additional information like the number of stars will appear once they are fetched. (We have a possible but unlikely case when the users can close the dialog before the handler code finishes to execute, but this will also be properly handled thanks to the underlying WPF implementation of the SynchronizationContext.) What is still worth changing?

  • [x] Getting data from the assembly can be done synchronously. It is artificially made async by using await Task.Run(() => assembly = Assembly.GetEntryAssembly()); Getting the entry assembly will not be a blocking IO especially compared to fetching data from the services. We can make the SetAssemblyDataAsync() async. (It could be left async for the sake of the symmetry with other methods, but simply return Task.CompletedTask.)
  • [x] The other three methods do not depend on each other and should be called in parallel and then awaited as one. See await Task.WhenAll().
  • [x] SetChangelogDataAsync() can either be done synchronously or made real-async. Similar to SetAssemblyDataAsync() the SetChangelogDataAsync() is artificially made async by using await Task.Run(() => markdown.Parse(File.ReadAllText("CHANGELOG.md"))); Again, compared to fetching data from the services, we can ignore those few milliseconds needed to make the file loaded and parsed. Or we can make the loading truly async by using async file loading

using (var reader = File.OpenText("CHANGELOG.md"))
{
    changelog = await reader.ReadToEndAsync();
}

• [x] Get the description from the assembly description (AssemblyDescriptionAttribute) instead from GitHub. As explained we assume that fetching data from GitHub will not always work and unlike the number of stars which can be ignored and not displayed we want the description to always be displayed.

• [x] Get copyright from the AssemblyCopyrightAttribute.

Coding Guidelines

• [x] Remove unused usings in:
  • [x] App.xaml.cs
  • [x] MainWindow.xaml.cs
  • [x] MarkdownToFlowDocument.cs
• [x] Remove unused XAML elements in:
  • [x] App.xaml
  • [x] MainWindow.xaml (in AboutDialog.xaml there is still xmlns:local="clr-namespace:Sharpen")
• [x] Remove useless comments in:
  • [x] App.xaml.cs (The standard, generated comment Interaction logic for App.xaml is still there and useles.)
  • [x] MainWindow.xaml.cs (Same like above.)
• [x] Have consistent tags closings on MainWindow.xaml. Sometimes it is "/>" and sometimes " />".
• [x] Remove this qualifier in MainWindow.xaml.cs where not needed.
• [x] Remove commented code from MainWindow.xaml.cs.
• [x] Write code comments as full sentences with punctuation e.g. ending with a dot.
• [x] Remove unused variable filePath in MarkdownToFlowDocument.

Use Consistent Naming

• [x] In App.xaml.cs:
  • [x] GitAuthToken -> GitHubAuthToken
  • [x] GitLogin -> GitHubLogin
  • [x] GitRepository -> GitHubRepository
  • [ ] VSMarketplaceID -> VSMarketplaceId (It is still ID instead of Id.)
• [x] Everywhere (e.g. in MainWindow.xaml) Github -> GitHub
• [x] In MainWindow.xaml.cs:
  • [x] ...Git... -> ...GitHub...
  • [x] ...ChangeLog... -> ...Changelog... (It is still fdsvVersionChangeLog -> fdsvVersionChangelog)
• [x] Use consistent casing in icon names consisting of two parts. E.g. Icon.github.png and Rating.FullStar.png is not consistent. Proposal is to go for the second, PascalCase, option and have Icon.GitHub and Icon.WWW.
• [x] Have more expressive names for images:
  • [x] Icon.png -> SharpenLogo.png
  • [x] PreviewImage.png -> SharpenLogo.Preview.png ??
  • [x] SharpenPackage.ico can stay as it is since it will be reused under that name once the dialog is added to the Sharpen project.
• [x] Rename MarkdownToFlowDocument to MarkdownToFlowDocumentConverter.
• [x] Rename MainWindow to AboutDialog.

Architectural Guidelines

• [x] Use Configuration Sections to pack together configuration settings used by the AboutDialog. Name the section AboutDialog. In the Lab, the dialog lives alone and having the settings as a plain non-grouped list is perfectly fine. But in Sharpen we will one day have several different configurations for different purposes and parts of Sharpen and we will need to group them in sections.
• [ ] Use MVVM with data binding. In this particular case, it means using data binding instead of accessing UI controls in code. For this relatively simple dialog accessing the controls directly is straightforward. But in general, we will have more complex UI in Sharpen and we want to use this Dialog for setting the guidelines for UI development. Basically, we want to use MVVM in a healthy combination with event handlers.

There are some other architectural considerations that will become relevant once we put the dialog into Sharpen. We will discuss them at that point.

Also, there will be some interesting extensions for the Markdown converter. But I like your approach to extend the parser in future iterations so we can discuss these extensions once the dialog is fully integrated into Sharpen.

Let me know if you have any additional questions.

[afrlan]

Hello!

I've implemented all but "Use MVVM with data binding."

Config sections seemed like an overkill; I think that prefixing keys with "AboutDialog." would accomplish the same grouping goal. However, since this is a learning exercise, I implemented it. (In production, type will be in another assembly, so "Sharpen.AboutDialog+ConfigSection, AboutDialog" will have to be changed accordingly.) Unfortunately, "The configuration section cannot contain a CDATA or text element." so something like

<aboutDialog>
    <gitHubAuthToken>value</gitHubAuthToken> 
</aboutDialog>

is not implementable (it seems).

I'll try to resolve data binding this week.

[ironcev]

Hi @afrlan,

Thanks for providing the updated PR!

As I mentioned before, I don't see MVVM as necessary to close this PR ;-)

The closed points are checked as done in my original comment. As you can see, some points are still left open. In my opinion, they need a bit more touch. In particular, I discuss in more detail the handling of the error case when fetching data from GitHub and VSMarketplace.

Also, in the previous feedback, I forgot to mention that the copyright, same as the description, should be taken from the assembly. So I've added that point to the original comment.

Please take a look at the extended original comment. The new remarks are written in italics.

Regarding the latest commits, here is my short feedback.

• [x] Instead of assembly.GetCustomAttributes(typeof(AssemblyDescriptionAttribute), false).OfType<AssemblyDescriptionAttribute>().FirstOrDefault(); we can use assembly.GetCustomAttribute<AssemblyDescriptionAttribute>();.
• [x] Rename ConfigSection to AboutDialogConfigurationSection and make it non-nested. (Also, remove unused usings from the class file.)
• [x] The new AboutLayoutTest lab does is not compliant with the lab guidelines. Also, it is not clear what is the purpose of that lab. It looks to me that it should be just an additional project in the AboutDialog lab. It is perfectly fine and actually very common to have several projects within a single lab.

[afrlan]

Hi,

Since I'm not used to proper commit, instead of resolving one issue at a time I fixed all and then made one commit. (I understand that many commits per issue are better than one commit per multiple issues.)

Other than that, I believe I fixed all unchecked boxes. Except databinding, that will be imlplemented later, when need arises (soon-ish, I expect).

Also, SomethingID will, probably, be the most difficult thing to change to SomenthingId.