Skip to content

Add Message tests#791

Merged
zsd4yr merged 5 commits intodotnet:masterfrom
hughbe:createparams-cleanup
Apr 19, 2019
Merged

Add Message tests#791
zsd4yr merged 5 commits intodotnet:masterfrom
hughbe:createparams-cleanup

Conversation

@hughbe
Copy link
Contributor

@hughbe hughbe commented Apr 15, 2019

  • Firstly, remove the dud methods UnsafeMethods.PtrToStructure/UnsafeMethods.StructureToPtr/UnsafeMethods.SizeOf as just call the equivalent Marshal method.
  • Use generic PtrToStructure
  • Make structCache.GetStruct generic to avoid so much casting
  • Refactor out WM_* and EM_* messages to avoid code duplication and extract them from the massive NativeMethods etc files and to match interop guidelines
  • Add unit tests for Message class, especially Message.ToString

@hughbe hughbe requested a review from a team as a code owner April 15, 2019 20:51
@codecov
Copy link

codecov bot commented Apr 15, 2019

Codecov Report

Merging #791 into master will increase coverage by 0.26702%.
The diff coverage is 66.13885%.

@@                Coverage Diff                 @@
##              master       #791         +/-   ##
==================================================
+ Coverage   27.70557%   27.9726%   +0.26703%     
==================================================
  Files           1061       1062          +1     
  Lines         291667     292311        +644     
  Branches       38514      38514                 
==================================================
+ Hits           80808      81767        +959     
+ Misses        206797     206487        -310     
+ Partials        4062       4057          -5
Flag Coverage Δ
#Debug 27.9726% <66.13885%> (+0.26702%) ⬆️
#production 18.33984% <54.3514%> (+0.19435%) ⬆️
#test 98.65933% <100%> (+0.01617%) ⬆️

@hughbe hughbe force-pushed the createparams-cleanup branch from 5cf50a5 to a767cd4 Compare April 15, 2019 21:53
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.

Thanks! I think we should consider making these enums in the future as I believe that would reduce the size of the assembly (see my inline comment).

Copy link
Member

Choose a reason for hiding this comment

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

This code makes me think it might be better to change all of these consts into enums and use reflection to get the name. I think overall we'd be reducing the size of the assembly by something measurable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zsd4yr I like this, and it wouldn't be too hard to do that in this PR.

One question - currently some window messages are commented out. Previously they'd return the hex value of the id in the Decode method, but now a string name would be returned. This actually seems quite good, right - is this a good change?

Copy link
Contributor

Choose a reason for hiding this comment

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

@hughbe, I am unfamiliar with the change you're asking about. Could you give me an example and let me know who the author is in the git blame? Has this been the case since we wen't open? If so, I can followup by "git blaming" in our previous system to discern why the change was made.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ignore me - I'm confused. Essentially I think @JeremyKuhne said that we should put this in an enum them use Enum.GetName(typeof(Interop.WindowMessages), id) in MessageDecoder. I pointed out that there isn't exact a one-to-one mapping between MessageDecoder and the values defined in Interop.WindowMessages or Interop.EditMessages

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 put this in an enum them use Enum.GetName(typeof(Interop.WindowMessages), id)

Yes, that is what I'm saying. It should cut the assembly size down. It's also nice when debugging to be able to see the name.

there isn't exact a one-to-one mapping

What doesn't match? We can update the names or special case.

@hughbe hughbe force-pushed the createparams-cleanup branch from e5edf7b to 6aa4d18 Compare April 16, 2019 18:27
@hughbe
Copy link
Contributor Author

hughbe commented Apr 16, 2019

Rebased - sorry for tall the force pushing :( Seems like there's been loads of PRs that sort of overlap but don't really that get in the way!

@hughbe hughbe force-pushed the createparams-cleanup branch from 6aa4d18 to 6b9cf6d Compare April 16, 2019 21:44
@zsd4yr
Copy link
Contributor

zsd4yr commented Apr 17, 2019

@hughbe would you mind adding a description with a summary of the changes you intend here?

Do we want to further separate out Native Methods and Unsafe Native Methods?

@sharwell sharwell mentioned this pull request Apr 17, 2019
@hughbe
Copy link
Contributor Author

hughbe commented Apr 17, 2019

Do we want to further separate out Native Methods and Unsafe Native Methods?

Yes! But thats going to be a gradual cleanup process

@hughbe
Copy link
Contributor Author

hughbe commented Apr 17, 2019

Rebased... again :((

@hughbe hughbe force-pushed the createparams-cleanup branch from bd6a20c to f681c43 Compare April 17, 2019 09:00
@zsd4yr zsd4yr merged commit 760d3e8 into dotnet:master Apr 19, 2019
@hughbe hughbe deleted the createparams-cleanup branch April 19, 2019 16:44
RussKie added a commit to RussKie/winforms that referenced this pull request Jun 28, 2019
As part the dotnet#791 refactor EM_* messages got mixed with RTB.EM_* messages
that led to an issue described in dotnet#994.

* Restore correct EM_ messagest
* Separate RTB.EM_ messages
* Update RichTextBox to use RTB.EM_* messages

Fixes dotnet#994
@ghost ghost locked as resolved and limited conversation to collaborators Feb 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants