search

ghaerr / microwindows

The Nano-X Window System
Other
659 stars 90 forks source link

More stuff for winextra #22

Closed roozbehid closed 5 years ago

roozbehid commented 5 years ago

More stuff for Windows and VisualStudio-Emscripten

ghaerr commented 5 years ago

Ok, it all looks pretty straightforward, no problems.

I do have a couple comments:

1) In osdep.c, you're now #if _MSC_VER > 15000... does that mean that this is equivalent to #if 0? If so, you can just go ahead and delete all that bracketed code, since my version doesn't need it. I don't want to keep it in there if we don't have to.

2) No big problem with removing WINAPI from Sleep(). Did you try seeing whether moving to SleepEx() in osdep.c would fix the problem? That has the advantage of allowing Win32 applications to call Sleep() in Microwindows and be exported should you or them someday need it.

3) In nx11/stub.c, it would be better not to #if 0 the IM routines. The reason that you're getting this is that you are including nx11/IM.c in your compile, you should just remove it. For that matter, you should remove the entirety of all nx11/*.c files from your build, they are only used for the X11 compatibility library, of which you will never use in your project.

The biggest issue by far is whether you need the hack code in osdep.c, (that is, the two areas _MSC_VER > 15000 like struct _SYSTEMTIME {... etc). I still don't know whether you need those duplicated structs and dllimports or whether you are including the real windows.h, which defines all of them. I don't need them in any of my Windows compiles because I am including osdep.c in a special project that compiles NOT using -Imicrowindows/src/include, thus using the real windows.h file.

Thanks, looks good!

ghaerr commented 5 years ago

The changes to drivers/scr_sdl2.c and include/mwtypes.h must be guarded with #if _MSC_VER > 1500, not #ifndef EMSCRIPTENT or unguarded. The SDL2 driver is shared on many systems and would immediately break the build if committed as written. The change to include/mwtypes.h (_CRT_DBG_MAP_ALLOC) should not be in a major header file. That change should be moved to include/uni_std.h where that header already handles allocation issues under #if _MSC_VER. Also, no need to also include in that file, you will notice that all files including uni_std.h already include .

roozbehid commented 5 years ago

So I think finally I am done :D

  1. Should be resolved. I removed them.
  2. Well I didnt do that, but it needs removal of VOID WINAPI Sleep from winmain.c. Maybe later? :D
  3. Fixed. Reverted. But I do want to use your X11 part in future. So soon for some other intresting stuff I will be using them.

I also reverted some CRTDebug stuff I added.

A new things that need your review are :

ghaerr commented 5 years ago

Can't accept the change from unsigned long UINT_PTR to unsigned int UINT_PTR, this breaks the wild for all 64 bit ports, including windows. This is because UINT_PTR must hold a pointer. If left as commit, we would need to have additional checks as to whether or not we're running on 32 bit or 64 bit system, none of which is currently needed now for 32 or 64 bit.

Does your system not work without this change?

ghaerr commented 5 years ago

*breaks the build, not wild, above.

roozbehid commented 5 years ago

I taught there was a case for 64bit. Reverted that.

ghaerr commented 5 years ago

In src/nx11/stub.c, where you're removing a previous #if 0, just delete your commit entirely rather than changing to #if 1. Same for any other changes that revert to no change. In that way I will be able to accept the whole commit when you ask for it.

Everything so far looks pretty good, with the exception of comments above. thanks!

roozbehid commented 5 years ago

you mean rewrite the git history to not include them? something like "git push -f" with new history?

ghaerr commented 5 years ago

Agreed we should change MK_LBUTTON etc to the values in real windows.h, and thus the base MWB UTTON_L should change also so that we don't have to do internal conversion. I rewrote the linux mice event driver a few weeks ago to handle this so shouldn't be any problems.

ghaerr commented 5 years ago

Regarding nx11/stub.c, no I don't mean remove the git history, but rather can I just delete that commit from your pull request entirely? How do I do that?

ghaerr commented 5 years ago

Ok I see, you've already beat me to it and removed everything. That works!

ghaerr commented 5 years ago

Ok so you ready for me to commit all this? I think we're good to go, I will take one more inspection pass when you're ready.

roozbehid commented 5 years ago

Yes. I think I am done for a while on this. Probably if something come up later I will go with another PR. But everything compiles fine on my side.

and thank you

ghaerr commented 5 years ago

Ok, I will merge your pull request now. I will be committing one more short commit afterwards with a quick update to ChangeLog and a few code comments.