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.

Sign up for GitHub

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

Jump to bottom

Add support for IFileOpenDialog #5319

Merged
merged 25 commits into from Jan 26, 2022
Merged

Add support for IFileOpenDialog #5319

merged 25 commits into from Jan 26, 2022

Conversation

kant2002
Copy link
Contributor

@kant2002 kant2002 commented Jul 22, 2021 •
edited

CCW Wrapper in the PR was autogenerated using source generator
and tailored based on my previous experience.
As such I have strange local variables sometimes.
Also I do not implemented anything yet for following interfaces

I plan to implement them, when collect all feedback on quality of this PR, so all improvements would be passed to next work.

What to do with code which maybe never called. How to detect that code? or should I bother with that and provide implementation for all methods?
For example I do not see errors in tests from Shell32.IFileDialogEvents, even if this interface used by VistaDialogEvents and passed to IFileOpenDialog.Advise method. So removing code and look where it fails in tests would be not practical.

Would like to ask question about IShellItem and IShellItemArray relationships. I will lookup on MSDN, but maybe collective conscionesness maybe help me better.
So question: Does these completely unrelated interfaces, or there some IShellItem implementation which implement IShellItemArray as well. Answer affects how I implement RCW for these interfaces.

Contributes to #5163

Microsoft Reviewers: Open in CodeFlow

@kant2002 kant2002 requested a review from a team as a code owner July 22, 2021 13:24
@ghost ghost assigned kant2002 Jul 22, 2021
@dreddy-work dreddy-work added the waiting-review This item is waiting on review by one or more members of team label Jul 22, 2021
@dreddy-work
Copy link
Member

@kant2002 , how do we test these changes?

@kant2002
Copy link
Contributor Author

@dreddy-work I assume some manual smoke test and writing more automated tests in separate PRs if needed. I would like to know what tests do you need to feel confident in the change?

@kant2002
Copy link
Contributor Author

MAybe problem that we do not detect issue lies in the fact that tests do not create IFileDialog See

winforms/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/FileDialogTests.cs

Line 701 in 94e03c4

private protected override IFileDialog CreateVistaDialog() => null;

@kant2002
Copy link
Contributor Author

List of cases which are not covered right now and which will fails at runtime.

FolderBrowserDialog dlg = new();
dlg.UseDescriptionForTitle = false;
dlg.Description = "some desc";
dlg.Show();

In general I do not see any tests which exercise opening actual FolderBrowserDialog.
Other case which I found

  1. launch WinformsControlsTest
  2. Press Dialogs
  3. Press 'Open dialogs'
    Crash immediately since it is set default folder, I do not cover that path.

I would like add these small tests somehere, but not sure what location best suited for that.

kant2002 added a commit to kant2002/winforms that referenced this pull request Jul 24, 2021
@kant2002
Add test for opening folder browser dialog
0793bdc 
In order to make writing tests morepleasant I add shortcuts for operations inside form
That allow me express tests better then just counting tabs and pressing enter.

This currently catches issue in dotnet#5319
@kant2002 kant2002 mentioned this pull request Jul 24, 2021
Merged
@kant2002
Copy link
Contributor Author

I do not get why Debug tests succeed. Release fails as expected in the new test which I add in #5344

@RussKie
Copy link
Member

RussKie commented Jul 26, 2021 

    System.Windows.Forms.IntegrationTests.WinformsControlsTest.WinformsControlsTest_OpenFolderBrowserDialogTest [FAIL]
      Assert.False() Failure
      Expected: False
      Actual:   True
      Stack Trace:
        /_/src/System.Windows.Forms/tests/IntegrationTests/System.Windows.Forms.IntegrationTests/WinformsControlsTest.cs(166,0): at System.Windows.Forms.IntegrationTests.WinformsControlsTest.WinformsControlsTest_OpenFolderBrowserDialogTest()

@RussKie
Copy link
Member

RussKie commented Jul 26, 2021

Have you tried running tests locally under Release?

@kant2002
Copy link
Contributor Author

Failure is expected, success is not. I rebase with test commit to test that we catch issue with prev implementation. I run under Debug locally and it fails, that's how I check that test catch what I want. Did not run under Release locally.

kant2002 added a commit to kant2002/winforms that referenced this pull request Jul 26, 2021
@kant2002
Reduce flackiness of the integration tests
1a1c3c8 
This number reduce count of times the test succeed when it should fail.
I assume this is because of WER doing it's job and process do not exit in time.
Related dotnet#5319
@kant2002 kant2002 mentioned this pull request Jul 26, 2021
Merged
@dreddy-work
Copy link
Member

@JeremyKuhne can you take a quick look at this change and share your feedback?

JeremyKuhne
JeremyKuhne reviewed Jul 27, 2021
View reviewed changes
src/System.Windows.Forms.Primitives/src/Interop/Interop.IID.cs Outdated
@@ -14,5 +14,14 @@ internal static class IID

// 7BF80980-BF32-101A-8BBB-00AA00300CAB
internal static Guid IPicture = new Guid(0x7BF80980, 0xBF32, 0x101A, 0x8B, 0xBB, 0x00, 0xAA, 0x00, 0x30, 0x0C, 0xAB);

// 43826D1E-E718-42EE-BC55-A1E261C37BFE
internal static Guid IShellItem = new Guid(0x43826D1E, 0xE718, 0x42EE, 0xBC, 0x55, 0xA1, 0xE2, 0x61, 0xC3, 0x7B, 0xFE);
Copy link
Member

Choose a reason for hiding this comment

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

Can you change these to internal static Guid IShellItem { get } = new(); please?

Copy link
Member

Choose a reason for hiding this comment

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

You still haven't changed these to properties as requested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I change only these who I add in this PR, should I modify others too?

Copy link
Member

Choose a reason for hiding this comment

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

If you could apply the change to all of these - it'd be great.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only IAccessible left, because it is used for ref Guid parameters. I think that maybe changed in follow-up PR if needed.

@JeremyKuhne
Copy link
Member

We probably shouldn't be taking changes of this nature into v6 at this point. We'll discuss the best way to handle making progress for v7 as a team and come back with info.

If you can clean up the local variable names and double check ref counting and allocations I'll take another look. Thanks!

@JeremyKuhne JeremyKuhne added the 📭 waiting-author-feedback The team requires more information from the author label Jul 27, 2021
JeremyKuhne
JeremyKuhne requested changes Jul 27, 2021
View reviewed changes
Copy link
Member

@JeremyKuhne JeremyKuhne left a comment

Choose a reason for hiding this comment

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

Please clean up the local variable names and take a look at the other feedback.

RussKie
RussKie reviewed Jan 19, 2022
View reviewed changes
Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

👍

src/System.Windows.Forms.Primitives/src/Interop/WinFormsComWrappers.cs
if (hr == S_OK)
{
Marshal.Release(externalComObject);
return new ShellItemWrapper(shellItemComObject);
}

throw new NotImplementedException();
Copy link
Member

Choose a reason for hiding this comment

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

Would we not want to call Marshal.Release(externalComObject); before throwing the exception?
Should we do this:

try
{
    // if (..) return ...
}
finally 
{
    Marshal.Release(externalComObject);
}

throw new NotImplementedException();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should not do that. If we reach this path, that's mean something within WinForms pass to internal ComWrappers borked value. That should not happens. That means serious bug for me which should be fixed.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed we have a bug, we thrown and the app crashed... But would we not leak the COM object in this situation? I'm genuinely curious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can leak COM object if it is created out-of-process. That may happens only if we work with WebBrowser or DataObject probably. All of them are problematic even for this approach and better discuss that problem when we implement this parts.

What if COM object would be passed by the user(this is how I think we end-up in this hypotetical scenario) if we release his object during exception(our bug) he maybe unhappy since that's leave him without workaround. I'm more in the camp let it fail since that means major screwup on our side.

src/System.Windows.Forms.Primitives/src/Interop/WinFormsComWrappers.FileOpenDialogWrapper.cs Outdated
Marshal.ThrowExceptionForHR((int)result);
}

pszName = Marshal.PtrToStringUni(pszName_local)!;
Copy link
Member

Choose a reason for hiding this comment

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

To confirm, even when the call fails we still get a non-null pszName_local buffer and we never return pszName as null?

@kant2002
Copy link
Contributor Author

To confirm, even when the call fails we still get a non-null pszName_local buffer and we never return pszName as null?

@RussKie now yes.

@dreddy-work dreddy-work added the 📬 waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) label Jan 20, 2022
JeremyKuhne
JeremyKuhne reviewed Jan 20, 2022
View reviewed changes
src/System.Windows.Forms.Primitives/src/Interop/Interop.IID.cs
@@ -9,13 +9,22 @@ internal static class IID
// 618736E0-3C3D-11CF-810C-00AA00389B71
public static Guid IAccessible = new Guid(0x618736E0, 0x3C3D, 0x11CF, 0x81, 0x0C, 0x00, 0xAA, 0x00, 0x38, 0x9B, 0x71);
Copy link
Member

Choose a reason for hiding this comment

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

We should have a follow up issue/PR to get this one fixed. Part of the idea here with using static properties is to avoid accidentally modifying these values. :)

JeremyKuhne
JeremyKuhne previously approved these changes Jan 20, 2022
View reviewed changes
Copy link
Member

@JeremyKuhne JeremyKuhne 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. If the dialog hasn't been manually tested it should be before checking in.

@kant2002
Copy link
Contributor Author

@JeremyKuhne Take a look at ac142c4 that's important point to consider. Also because System.Windows.Forms.IntegrationTests project seems to be did not run, it fails to catch crash. I do know what to do, and will fix it in subsequent commit. Please do not merge meanwhile.

@kant2002
Perform unwrapping of CCW to IntPtr inside ComWrapper
e15e540 
That, and introducing `SHCreateShellItemViaComWrappers` make Folder browser dialog works. `SHCreateShellItemViaComWrappers` is temporary means until I walk through all paths and replace usages of old function which rely on built-in COM one by one. That's should be easier for me tot test, and for you to review.
@kant2002
Copy link
Contributor Author

@JeremyKuhne @RussKie in last commit I add unwrapping RCW and that make things working. I validate changes manually by running WinformsControlsTest and opening dialog, made a selection and changing properties a bit. I cannot say that I test exhausively. I would like to add test coverage in form of E2E tests, so I file #6532 . In additional to other small work-items in this PR, I think nothing blocks this from landing (unless you raise concerns over last 2 commits)

@RussKie
Copy link
Member

RussKie commented Jan 24, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@kant2002
Copy link
Contributor Author

@RussKie anything else left?

@RussKie
Copy link
Member

RussKie commented Jan 25, 2022

I'm OOF. @dreddy-work @JeremyKuhne please merge , if you have no further feedback. Thanks.

@dreddy-work dreddy-work added the ready-to-merge PRs that are ready to merge but worth notifying the internal team. label Jan 25, 2022
@dreddy-work dreddy-work merged commit fc4f862 into dotnet:main Jan 26, 2022
@ghost ghost added this to the 7.0 Preview2 milestone Jan 26, 2022
@kant2002 kant2002 deleted the kant/comwrappers2 branch January 26, 2022 19:22
@kant2002
Copy link
Contributor Author

Ooh!! Yeah!!! Thanks you guys a lot!!!

@ghost
Copy link

ghost commented Jan 26, 2022

Hi @kant2002, it looks like you just commented on a closed PR. The team will most probably miss it.
If you have a question - consider opening a new discussion thread. Alternatively, you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

This was referenced Jan 27, 2022
Open
Merged
Merged
@dotnet dotnet locked as resolved and limited conversation to collaborators Mar 1, 2022
@RussKie RussKie removed ready-to-merge PRs that are ready to merge but worth notifying the internal team. 📬 waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) labels Oct 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@RussKie RussKie RussKie left review comments

@AaronRobinsonMSFT AaronRobinsonMSFT AaronRobinsonMSFT left review comments

@JeremyKuhne JeremyKuhne JeremyKuhne approved these changes

Assignees

@kant2002 kant2002

Labels
None yet
Projects
None yet
Milestone
7.0 Preview2
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@kant2002 @dreddy-work @RussKie @JeremyKuhne @AaronRobinsonMSFT