Closed leok7v closed 7 years ago
Thanks. Will review it at some point.
Excellent! I see two issues, it seems you've missed this file on your patch and also you need to apply this:
diff --git a/demo/glyphy-demo.cc b/demo/glyphy-demo.cc
index 9ff7ce0..6a2a0ad 100644
--- a/demo/glyphy-demo.cc
+++ b/demo/glyphy-demo.cc
@@ -190,7 +190,7 @@ int
main (int argc, char** argv)
{
/* Process received parameters */
-# include "jabberwocky.h"
+# include "default-text.h"
const char *text = NULL;
const char *font_path = NULL;
char arg;
@@ -222,7 +222,7 @@ main (int argc, char** argv)
text = argv[optind++];
else
// text = jabberwocky;
- text = usage;
+ text = default_text;
}
if (!font_path || !text || optind < argc)
{
And now it works!
@ebraminio can you please provide a combined pull request? Possibly address my comments in review of https://github.com/behdad/glyphy/pull/12 ? Thanks!
Will try to do it ASAP. Tomorrow or Thursday. Have release at work.
On Tuesday, July 12, 2016, Behdad Esfahbod notifications@github.com wrote:
@ebraminio https://github.com/ebraminio can you please provide a combined pull request? Possibly address my comments in review of #12 https://github.com/behdad/glyphy/pull/12 ? Thanks!
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/behdad/glyphy/pull/18#issuecomment-232249546, or mute the thread https://github.com/notifications/unsubscribe/ABGKBas1DLKXTazcBKlD8JLfThVvq3KDks5qVGBrgaJpZM4IwZIK .
Leo
Done. Let me know if anything else needed.
I still can't find glyphy-windows.h
in your pull request. Perhaps you need to do git add
and commit it.
added
Leo
On Fri, Jul 15, 2016 at 1:36 PM, Ebrahim notifications@github.com wrote:
I still can't find glyphy-windows.h https://github.com/tml1024/glyphy/blob/master/src/glyphy-windows.h in your pull request. Perhaps you need to do git add commit it.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/behdad/glyphy/pull/18#issuecomment-233063381, or mute the thread https://github.com/notifications/unsubscribe-auth/ABGKBesp4estSfCJmzcl7Kbw4SO6gPpQks5qV-8_gaJpZM4IwZIK .
Addressing some of Behdad's comments on old patch would be nice also in the mean while :) First one means defining demo_face_t one with FT_Face or HDC and using it on other places (instead doing that everytime). Second one seems to be just a rename. Not sure about Third one I think better to keep it on this way as our friend GDI is supposed to remain on Windows for what I see, I didn't tried forth one to see how much hassle it needs :)
About your changes generally, if I were you, I would've used cmake build script to generate VS project files and download dependencies on demand, like my pet project as your approach has increased glyphy repo clone size from 1.56 MiB to 2.30 MiB but I think thats negligible due the easiness it provides :) LGTM from me anyway but Behdad should review, approve and merge it.
I could work on it more and clean it up more. And I will. I agree with all proposed changes including cmake. But I needed working base ASAP and in a simplest way and without external libs dependencies. Cleanup is easier after that.
That said my time is limited and I can only spend few hours a week/month working on it. If we merge now - maybe somebody will pick it and clean it further.
If not - it's OK too. It's still available in my clone which I will try to keep in sync and further improvements can be done there.
On Friday, July 15, 2016, Ebrahim notifications@github.com wrote:
Addressing some of Behdad's comments on old patch https://github.com/tml1024/glyphy/commit/cfc3157868f691b70c2f0a6daa3c387ca6ef42a9 would be nice also in the mean while :) First one https://github.com/tml1024/glyphy/commit/cfc3157868f691b70c2f0a6daa3c387ca6ef42a9#commitcomment-14647074 means defining demo_face_t one with FT_Face or HDC and using it on other places (instead doing that everytime). Second one https://github.com/tml1024/glyphy/commit/cfc3157868f691b70c2f0a6daa3c387ca6ef42a9#commitcomment-14647093 seems to be just a rename. Not sure about Third one https://github.com/tml1024/glyphy/commit/cfc3157868f691b70c2f0a6daa3c387ca6ef42a9#commitcomment-14647114 I think better to keep it on this way as our friend GDI is supposed to be here, I didn't tried forth one https://github.com/tml1024/glyphy/commit/cfc3157868f691b70c2f0a6daa3c387ca6ef42a9#commitcomment-14647105 to see how much hassle it needs :)
About your changes generally, if I were you, I would've used cmake build script to generate VS project files and download dependencies on demand, like my pet project https://github.com/ebraminio/glcourse/blob/master/init.bat as it increased glyphy repo clone size from 1.56 MiB to 2.30 MiB but I think thats negligible due the easiness it provides :) LGTM from me anyway but Behdad has to approve and merge it.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/behdad/glyphy/pull/18#issuecomment-233073429, or mute the thread https://github.com/notifications/unsubscribe-auth/ABGKBSM7xgUWfBinGk82F79c2EOwgb_Gks5qV_pygaJpZM4IwZIK .
Leo
I guess https://github.com/Microsoft/vcpkg would be useful for this now as it reduces lot of hassles we have around pulling dependencies
^ I'll try to do it and send pull request to your repo if you agree with its use :)
Anything that doesn't put a lot of junk in glyphy repo is doable.
Sure. Good idea.
Leo
On Thu, Sep 22, 2016 at 11:22 PM, Ebrahim Byagowi notifications@github.com wrote:
^ I'll try to do it and send pull request to your repo if you agree on its use :)
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/behdad/glyphy/pull/18#issuecomment-249112440, or mute the thread https://github.com/notifications/unsubscribe-auth/ABGKBSt5ueCyWR_MfPeYAIMKUTAB_lyjks5qs3BAgaJpZM4IwZIK .
Very sorry, I thought you are very AFK so I sent a separate pull request https://github.com/behdad/glyphy/pull/19 I tried to attribute you guys on my patch but if you think that is not enough please let me know. I also wrote a CI config and a JScript code for it to avoid need of external tools, have a look please if you got sometimes.
I don't need attribution. Go ahead. Yes I've read JavaScript. It is OK problem is you never know when Microsoft kill file system ActiveX control. Just a thought: why don't just stringify shaders and check them in. I know fixing something in the stringifed code will be just a bit more hussle but not by much. And this will completely eliminate clumsy stringify phase.
Shaders don't change that often.
On Saturday, October 1, 2016, Ebrahim Byagowi notifications@github.com wrote:
Very sorry, I thought you are very AFK so I sent a separate pull request d54fd36 https://github.com/behdad/glyphy/pull/19/commits/d54fd367f535c4900eb641c2d0f0050dea3a0eb0 I tried to attribute you guys on my patch but if you think that is enough please let me know. I also wrote a CI config and a JScript code for it to avoid need of external tools, have a look please if you got sometimes.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/behdad/glyphy/pull/18#issuecomment-250928312, or mute the thread https://github.com/notifications/unsubscribe-auth/ABGKBXjXsOOmQXAiBoL1l02_A6B5ATdGks5qvqTjgaJpZM4IwZIK .
Leo
In favor of #19
I don't need attribution. Go ahead.
Thank you, I did it anyway, I guess :)
Yes I've read JavaScript. It is OK problem is you never know when Microsoft kill file system ActiveX control.
I hope they won't do it till their next version of Windows :)
Just a thought: why don't just stringify shaders and check them in. I know fixing something in the stringifed code will be just a bit more hussle but not by much. And this will completely eliminate clumsy stringify phase.
I agree also, not only about this, but also about .rl files of harfbuzz project which needs ragel tool that is not easy to download on Windows. But that's life, I guess :)