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 WinFile-specific about dialog box #76

Closed
wants to merge 2 commits into from

Conversation

matthewjustice
Copy link
Contributor

As recommended in issue #40, an "About" dialog was added that is specific to the application. The dialog is simple; it could be expanded upon as needed.

One note of interest: winfile.h already contained a function prototype named "AboutDlgProc", seemingly a dialog proc callback for an "About" dialog, but the code did not contain an implementation of that function. I suspect that the code previously had such a function and it was removed, or it was never implemented. Regardless, I left that prototype in place and implemented the DialogProc callback function using the same function name and signature.

@msftclas
Copy link

msftclas commented Apr 11, 2018

CLA assistant check
All CLA requirements met.

Copy link
Contributor

@NazmusLabs NazmusLabs left a comment

Choose a reason for hiding this comment

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

I'd change the line: "Based on Windows NT 4.0." To "Version 10.0. (C) Microsoft Corporation, All rights reserved.
And replace the next line with "Licensed under the MIT License (INSERT GITHUB LINK)"

This is a more standard practice for software about pages. The version number of the software, the copyright holder, and the license information. None of these information are currently included in this about dialog box.

@matthewjustice
Copy link
Contributor Author

@NazmusLabs - That is good feedback; thanks! Let me speak to the specific points you raised:

  • Version number, agreed that the about box should display it. However, I didn't want to hard-code it as a string. I'd rather see a major, minor, revision, build number in the binary, which would show up in the file properties as well as the about box. I didn't see such a versioning pattern already in place (maybe I missed it), so adding versioning seemed like a bigger effort for later.
  • "(C) Microsoft Corporation, All rights reserved." - That does seem like standard text, but I was hesitant to make copyright claims without some input from Microsoft. @craigwi - Do you have any feedback on this point?
  • Calling out the MIT License makes sense; good idea. @NazmusLabs were you recommending a link to the MIT license itself here or a link to the winfile project?

@craigwi
Copy link
Contributor

craigwi commented Apr 11, 2018

Hi @matthewjustice , thanks for working on this. The Copyright text should match what is in the source files:

Copyright (c) Microsoft Corporation. All rights reserved.
Licensed under the MIT License.

Definitely need a versioning number scheme. I'm open to ideas.

@NazmusLabs
Copy link
Contributor

@matthewjustice I got the copyright information from the License file both on the github and also included with the release binary :)

And for versioning, there isn't any version information in code because it relies on the NT version, which is why just displays the Winver dialog box. I believe Windows 3.1's version of WinFile did have its own about box. Was probably removed in 9x or NT.

I suggested 10.0. Rather than 10.0.0 for this very reason. The numbers after the minor version are typically build and revision numbers. And if I were going to hard code a version number, I'd just put Version 10, or version 10.0. It'll be a while before we'd consider calling it Version 10.1 or 11. So I thought hard coding version 10.0 for now was a safe bet.. By the time we have enough features to warrant a v10.1 release, I assumed we'd have a proper version string and the about box would have been updated.

@matthewjustice
Copy link
Contributor Author

matthewjustice commented Apr 11, 2018

@craigwi and @NazmusLabs - Yes, makes sense to reflect the copyright and license that is in the source and release binary. I'll make that change.

As for versioning, yes, I understand that Windows binaries typically reflect the version of the Windows build, so it doesn't make sense to version them independently. Now that this code is independent from the Windows source, I'd like to see a versioning scheme in place, but that feels like a separate issue.

At this point, for the about box specifically, I can either:
a) hard-code version 10.0 as a string, with the expectation that it will be updated later.
or
b) query the file version information (VS_FIXEDFILEINFO) and display that so that when the values are actually updated they will show up. I could limit this to major and minor fields only for now, so it would still show 10.0.

Unless there are objections to doing so, I can submit a PR soon with a hard-coded "10.0" in the UI, and, time-permitting, submit another PR that grabs the version number from VS_FIXEDFILEINFO in the binary.

@matthewjustice
Copy link
Contributor Author

matthewjustice commented Apr 11, 2018

I've made two changes to this PR:

  1. The text is updated as discussed. See screenshot below.
  2. I'm now handing IDCANCEL in the dialog proc in case the user clicks the X in the upper right. That should have been there before.

about2

Some things to note:

  • I did go with the hard coded "10.0" string for now, but using the VS_FIXEDFILEINFO approach is something I (or someone else) should consider for later, along with general versioning strategy.
  • The GitHub URL isn't a hyperlink. Adding a SysLink requires some other changes and I wanted to keep things simple.

@NazmusLabs
Copy link
Contributor

The dialog looks great! And what would happen if the user clicked on the x before, haha.

And, right, I forgot to ask about the link. I was going to ask that I didn't believe that COM APIs WinFile is using had support for hyperlinks within in dialog boxes. I remember that becoming a thing with dot net apps and then later with Vista, where native Windows dialog boxes has better support for hyperlinks.

I think it's be easier, for now, to have a button beside the "OK" button labeled "License" that loads the local license text file that's includes with the app.

@matthewjustice
Copy link
Contributor Author

Thanks @NazmusLabs! If the user clicked the X before, nothing would happen, so not a great user experience! Yes, dialog hyperlinks via SysLink haven't always been there; the SysLink control is in ComCtl32.dll v6, and requires a manifest declaration. I like your license button idea... maybe something for a future PR?

craigwi added a commit that referenced this pull request Apr 12, 2018
made version text settable; based on PR #76.
@craigwi
Copy link
Contributor

craigwi commented Apr 12, 2018

nice work; many thanks! Made a few tweaks and added the about box.

@craigwi craigwi closed this Apr 12, 2018
@craigwi craigwi added the fixed differently Spirit of PR was taken if not all of the details; credit given to original author. label Apr 12, 2018
@matthewjustice
Copy link
Contributor Author

matthewjustice commented Apr 12, 2018

@craigwi - I'm glad to help! One small thing I noticed about one of the tweaks, the WM_INITDIALOG code path will result in AboutDlgProc returning FALSE, which in theory will "prevent the system from setting the default keyboard focus" (MSDN). Now, in practice, it doesn't seem to matter, and the code works fine.

@matthewjustice matthewjustice deleted the about-dialog branch April 12, 2018 04:19
@craigwi
Copy link
Contributor

craigwi commented Apr 12, 2018

Thanks; I'll fix that.

@matthewjustice
Copy link
Contributor Author

@craigwi - FYI PR #89 made changes in the WM_INITDIALOG code, so I added a fix for the return issue mentioned above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed differently Spirit of PR was taken if not all of the details; credit given to original author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants