Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup: More documentation fixes #8208

Merged
merged 2 commits into from Jun 9, 2020
Merged

Conversation

techgeeknz
Copy link
Contributor

@techgeeknz techgeeknz commented Jun 8, 2020

This is in preparation for some major refactoring of the dirty block system (not to be confused with the dirty rectangle system) that is used by gfx.cpp to track portions of the screen that have been invalidated but not yet redrawn (at least, that is how I think it works; still investigating this).

That, in itself, is in preparation for moving the responsibility of drawing the mouse pointer (and, probably, the "network chat message"; whatever that is) into the video driver, so it can be updated independently of the rest of the screen (moving toward fixing #7006).

Copy link
Contributor

@Yexo Yexo left a comment

Choose a reason for hiding this comment

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

Happy to approve the first commit, but not the 2nd one as it stands.

src/window.cpp Outdated
@@ -3478,6 +3478,8 @@ void ReInitAllWindows()
* @param clss The class of the window to position.
* @param setting The actual setting used for the window's position.
* @return X coordinate of left edge of the repositioned window.
*
* @todo Replace \c setting magic numbers with an enumeration?
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like todo's like this. I agree that the function would be cleaner without the magic numbers, but it's relatively easy to make lots of todo's like this that don't add any value but distract from the actual code. For small cleanups like this I'd rather leave out the todo than add it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Point taken. The code is probably already littered with too many "TODO" and "XXX" comments, so I'll try to keep them to myself in future. At the very least, we can take solace in the fact that one of the previously documented todos has actually been done :)

Copy link
Contributor

@Yexo Yexo left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution :)

@LordAro LordAro merged commit ee570e1 into OpenTTD:master Jun 9, 2020
@techgeeknz
Copy link
Contributor Author

Thanks for your contribution :)

You're more than welcome, master.

@LordAro
Copy link
Member

LordAro commented Jun 10, 2020

You should stop with the "master" stuff. It's weird...

@techgeeknz
Copy link
Contributor Author

You should stop with the "master" stuff. It's weird...

Sorry, I thought that's how we did things around here. On my first pull request, someone implied I should thank the person who merged it in.

@LordAro
Copy link
Member

LordAro commented Jun 13, 2020

"master" is the name of the branch it's being merged into, not the name of the person who did it...

@techgeeknz
Copy link
Contributor Author

"master" is the name of the branch it's being merged into, not the name of the person who did it...

I thought it could also be a noun by proxy for the person who is merging to master. Regardless, I'll stop with the "weirdness". 😉

@andythenorth
Copy link
Contributor

Ha ha, that 'say thanks master' was in some very random looking comments from kawayanslayer.

No idea who / what kawayanslayer is, looked like someone testing an email bot to me, or it was just spam. 😄

Anyway, you can ignore that 😉

@techgeeknz
Copy link
Contributor Author

Thanks, master. Okay, I'll stop now 😎.

@techgeeknz
Copy link
Contributor Author

techgeeknz commented Jun 13, 2020

It also seems rather coincidental that @kawayanslayer joined GitHub on that very same day and posted an initial one-line commit to a repository they created 🤔.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants