search

Robmaister / SharpFont.HarfBuzz

HarfBuzz bindings for C#
MIT License
19 stars 8 forks source link

GlyphPosition + GlyphInfo #3

Closed ebraminio closed 10 years ago

ebraminio commented 10 years ago

Honestly I failed to find sn.exe on my Visual Studio and on Developer Command Prompt also am not sure this method of marshalling is right, 20 is just sizeof hb_glyph_info_t and hb_glyph_position_t and I don't know how to retrieve it with some cleaner way but not defining private variables of them on csharp. I am aware this design is more similar to the original C API but I am not sure how a redesign in C# way would look like, however with this informations porting or using it on this example would be easy, I hope you could get some time and have a look.

Robmaister commented 10 years ago

I won't merge this in it's current state. The formatting of the code should match mine for both SharpFont and SharpFont.HarfBuzz, so hard tabs with a tabstop of 4 instead of soft tabs. In Visual Studio this would be Tools -> Options -> Text Editor -> C# -> Tabs. Make sure "Keep tabs" is selected.

Half of your changes are converting hard tabs to soft tabs, which serves no purpose and makes using tools like git blame more annoying.

Also your reference to SharpFont in the .csproj file somehow switched to the obj/ folder from bin/. It should stay as bin, and I should write clear instructions to submodule update then build SharpFont when you first checkout this repo, or include it as a pre-build step or something.

Change those two things (and also convert your other pull request to hard tabs if you wrote them as soft tabs) and I'll merge it in.

Thanks for taking the time to actually write this code, though!

Robmaister commented 10 years ago

Additionally, sn.exe "just works" on my laptop so I could go ahead and do the strong-name signing and add the internalsvisibleto property after work today.

ebraminio commented 10 years ago

Sure! Thanks for the review! :)

ebraminio commented 10 years ago

I don't know, may they removed it from Visual Studio 2013?

ebraminio commented 10 years ago

Unfortunately I did two commit here so couldn't do amend on them so I've done as a separate commit, please let me know if you like to make all of this pull request as a one commit and I would abandon this and redo these.

Robmaister commented 10 years ago

It would be awesome if you could do that, yeah. Not sure how GitHub pull requests handle squashing commits, but try doing this:

http://blog.steveklabnik.com/posts/2012-11-08-how-to-squash-commits-in-a-github-pull-request

Robmaister commented 10 years ago

Also I'm using VS2013 (and just recently upgraded to Win 8.1, so there isn't an old install of 2012 in there), for some reason the VS cmd is named "VS2012 x86 Native Tools Command Prompt" and running sn or sn.exe work.

ebraminio commented 10 years ago

Done. But I don't have it :-(

Robmaister commented 10 years ago

That is probably related to the error at the top of the shell: ERROR: Cannot determine the location of the VS installation, which means it won't add any of the Visual Studio tools to PATH.

http://social.msdn.microsoft.com/Forums/vstudio/en-US/78703f6b-f610-456c-b770-76a12be3e1ae/error-cannot-determine-the-location-of-the-vs-common-tools-folder?forum=vssetup

Also don't worry about it, I'll set it up myself right now.

And thanks for fixing the PR!

ebraminio commented 10 years ago

Oh. How I didn't notice that! Thank you

Robmaister commented 10 years ago

Alright, SharpFont.HarfBuzz is strong-name signed and SharpFont has a InternalsVisibleTo property on the assembly. Make sure to recompile the submodule (and update? don't know if git does that automatically or not when you pull) to see the changes