mochadoom icon indicating copy to clipboard operation
mochadoom copied to clipboard

Fix some bugs mostly related to classic/vanilla Doom behavior

Open gaborbata opened this issue 2 years ago • 4 comments

  • Fix vanilla mouse event handling (in game, in menu)
  • Fix initial value of novert (i.e. disable vertical movement by default)
  • Fix fullscreen resize options (Nearest, Bilinear, Bicubic)
  • Fix writing alternate config files i.e. when -config argument is used
  • Fix/suppress some compile warnings, remove deprecated API usage
  • Show message dialog on missing WAD files
  • Improve logging
  • Improve WAD handler cleanup
  • Center application window
  • Minor code cleanup and formatting

gaborbata avatar Oct 10 '23 11:10 gaborbata

I'm looking at the changes this time, and unlike the previous MR, there are just formatting changes to files, like replacing tab indentation with spaces. GitHub displays 425 changes. This obfuscates your functional changes and it makes it hard to see which files are really affected. I don't like reformatting, but I'll accept code cleanup if it's done manually and not by a tool that does it automatically. Can you avoid formatting changes? It makes MRs hard to review. I prefer atomic commits:

Atomic commits:

  • Only affect a single aspect of the system.
  • Allow for greater understandability of changes
  • Less effort to roll back changes when needed
  • Easier bug identification

Your changelog is already well separated into different bullet point. Your commits should be the same. So one commit that does "Fix vanilla mouse event handling", one that's "Fix initial value of novert", another one that's "Fix fullscreen resize options", etc. One commit per bug fix, improvement or new feature. This would allow me to review much quicker.

AXDOOMER avatar Oct 10 '23 14:10 AXDOOMER

Thanks for your comments. Will try to separate the PR to multiple commits soon.

(Please note that GitHub can ignore whitespaces during reviews)

gaborbata avatar Oct 10 '23 17:10 gaborbata

Hey, @AXDOOMER I've split the changes into separate and meaningful commits.

Unfortunately, I can't avoid the formatting changes (401d4ed) at that point easily, however this is a separate commit. I think a consistent (and standard) code formatting can help understanding and maintaining the code (and even to avoid some bugs).

gaborbata avatar Oct 11 '23 11:10 gaborbata

Sorry for not having merged this yet. I haven't forgotten about it and it will get merged within the next few months. I'm just too busy with full-time work + full-time studying.

AXDOOMER avatar Feb 06 '24 13:02 AXDOOMER