ghaerr / microwindows

The Nano-X Window System
Other
648 stars 91 forks source link

Win32 features #54

Closed djipi closed 3 years ago

djipi commented 3 years ago

Makefile update, some defines, Atari Jaguar fix and a new timeGetTime function.

ghaerr commented 3 years ago

Hello @djipi,

Most of this looks good, except for two items:

The first item is very strange, the strncpy/strzcpy fix in winfont.c... I already made these changes in the repo, with my PR #53. But yet GitHub is showing there is no conflicts with the branch! Perhaps @rofl0r can understand, I presume this is still something from the mess-up with the force-commit. Please check this, as it seems the change is already in the repo. This could also be a result of your repo still being out of sync with master as the result of the force commit. I don't know.

The other issue is the new timeGetTime function. I have a couple issues with it: 1) what use is it, it is not being called from within Microwindows at all, so it should not be in osdep.c. 2) Almost all the code is duplicated from GdGetTickCount, and so that's not a good idea. 3) Looking closer, I see that perhaps you have a need for a timer function that is in 1ms, rather than 25ms resolution. This could be fixed possibly be changing GdGetTickCount, rather than introducing a new function. Finally, I realize that many of the operating systems in GdGetTickCount are coded incorrectly, since the whole reason GdGetTickCount was coded to subtract startTicks was to ensure that the "time" didn't overflow back across 0, as that will cause problems with its use internally.

Let me know what you think and we will move forward. I am sorry about the git troubles.

djipi commented 3 years ago

Hi @ghaerr ,

About the PR #53, I have done the merge in my master branch before, and deleted my branches used by previous PR; I have checked the best I could and cannot find sync problems. I do not understand yet why this commit keep coming.

You are correct, I have done the timeGetTime function for the 1ms granularity. I have not touched the GdGetTickCount function for the another operating systems. I cannot be very helpful here.

All in one, I feel this PR is not that useful, except may be for some defines. I will probably recommend to ignore this PR. Sorry about the trouble, better luck next time.

ghaerr commented 3 years ago

If you like, I can add all the other items, other than the time function and the strzcpy, on my side, as I think they are useful.

I will also look into moving to 1ms granularity, I suspect I added it because Win32 itself uses 25ms, and I was trying to be identical, but will have to confirm.

I can't yet determine what the git sync problems are, as I am still traveling and pulled a completely new clone on my laptop after @rofl0r did the force-push. I will have to see if there are problems on my main system repo back home in a week or so.

You might need to create a new clone to fix this git problem, then use the method @rofl0r stated to get all your changes out of your old tree branches. I am sorry - at this point I think it was a mistake to do the forced git push, given what little I know about git, so I can't help you much directly.

djipi commented 3 years ago

Sure, please add the useful items. In case of I wish to keep the timeGetTime function, where will be the best place to add it? Winextra?

rofl0r commented 3 years ago

The first item is very strange, the strncpy/strzcpy fix in winfont.c... I already made these changes in the repo, with my PR #53. But yet GitHub is showing there is no conflicts with the branch! Perhaps @rofl0r can understand, I presume this is still something from the mess-up with the force-commit. Please check this, as it seems the change is already in the repo.

no, this is unrelated to that. git's merging mechanism is a bit odd, i don't understand why in this case no conflict is shown. however this can easily be fixed by deleting the "string manipulation fix" commit on @djipi 's side: git rebase -i HEAD~2 on this branch, then in the editor that pops up delete the line with the string manip commit, then force-push the branch.

rofl0r commented 3 years ago

I can't yet determine what the git sync problems are, as I am still traveling and pulled a completely new clone on my laptop after @rofl0r did the force-push. I will have to see if there are problems on my main system repo back home in a week or so.

i think the error was the commit when @djipi merged master into his branch ( https://github.com/ghaerr/microwindows/commit/1450f15470e9680d6be33456d1008808b2720b28 ), this is pretty bad style. one should always do work in a feature branch based on upstream master, and never touch own master branch apart from pulling offstream as a fast-forward (i.e. no merge commit is created), and then rebasing the feature branch on master as needed. btw, it seems this PR is already merged as i see the commit "string manipulation fixes" in the commit list. this is probably a different change but with the same commit message

ghaerr commented 3 years ago

@rofl0r,

Thanks for the explanation.

Now, I always do the same for other repositories, on in my account, but what should I do about Microwindows? In the old days, I would just push directly onto master with no PR (I know, very bad). Now, I create a separate branch, commit it (to my own repository, which is ALSO the upstream master), then do a merge commit for the PR. Should I be using the "Rebase Commit" GitHub option for my own commits?

rofl0r commented 3 years ago

whenever a merge commit is created, one gets a non-linear git history. i really dislike that because it makes it hard to impossible to process the history with automatic tools (e.g. https://github.com/dino-lang/dino/issues/17#issuecomment-565777628) and additionally it results in confusing situations like this.

therefore i consider it essential to have a local git frontend like gitk that visually displays what happens. here's a screenshot of this repo

Alternate image text

here's one of my own repos Alternate image text

as you can see, the line with the dots on the left is entirely flat.

using a git repo without such a frontend is like climbing down from a mountain in complete darkness.

rofl0r commented 3 years ago

I would just push directly onto master with no PR (I know, very bad)

nothing bad about that, since this is your repo :)

... then do a merge commit for the PR. Should I be using the "Rebase Commit" GitHub option for my own commits?

yes, the "rebase and merge" option is much cleaner as it prevents the nasty merge commits from happening.

rofl0r commented 3 years ago

at this point I think it was a mistake to do the forced git push

the more time passes between a push and a force-push ironing out a mistake, the higher the chances that someone else pulls in that time window and has to fix his local clone. that's why it is generally considered rude to do that, and why i stayed up like 2 hours in the night it happened to assist that it gets done quickly. even when i came back to the repo after 3 days i first checked the traffic tab to see there were only 3 clones since it happened, so it appeared reasonable.

usually i only do a force-push to a master branch when i notice immediately (say during 15 mins) that an error happened or if it is a really grave mistake (like mistakenly checking in a 2 MB big binary blob).

djipi commented 3 years ago

Hi @ghaerr Sorry, I've forgotten to close this PR.

ghaerr commented 3 years ago

Hello Jean-Paul,

Thanks - I was going to add the rest of the changes that you had originally submitted, before the tree was force-pushed, but I never got around to it. Should you pull another tree and want to submit PRs for those changes, I would be glad to accept them, especially for those around the hardware requirements of 8-byte padding.

Thank you!