Game information in-game, layout options, fixes, architecture updates

nullpainter commented 5 years ago

Feature changes

Added MAME .lay creator and dynamic game title image renderer for displaying in-game game information.
Replaced game list rebuild loading animation with progress bar.
Re-added MAME logo, using latest official logo.
Re-added splash screen.
Displayed MAME logo while MAME is loading, even if splash screen is disabled.
Updated mirroring to mirror logo/splash screen to all monitors.
Added button to reset configuration to defaults (excluding MAME executable path).
Made OK/Cancel buttons consistent with Windows UI (order of buttons, case of OK button).
Allowed non-game configuration changes to be made while rebuilding game list.
Changed icon to be MAME's 'M' for tighter branding.
Capitalised MAME text for same reasons.
Updated documentation.
Added layout configuration options:
- Splash screen toggle
- Splash screen font face and duration
- In-game title toggle
- In-game title font face and font size
Add advanced configuration options:
- Skip game validation (for custom MAME builds)
- Always write debug logging to disk, even in release builds

Under the hood changes

Replaced tight loop for splash screen display with a timer.
Shuffled games instead of choosing new game via Random. The previous use of Random wasn't particularly good anyway, as it was re-creating the object each time instead of using a single instance. Shuffling means that users aren't going to get the same game consecutively, and allows us to navigate to next/previous game when the hot key feature is developed.
Fixed condition where interrupting screensaver while MAME was loading would prevent MAME from actually exiting.
Added logging.
Fixed some logging
Updated logging to use parameter lists as recommended for Serilog
Reinstated dialog if error running MAME during screensaver.
Tidied code.
Added comments.
Prevented crashes if quitting screensaver during splash screen.
Added dependency injection via Simple Injector (see below)
Changed registry-based configuration to XML (see below)
Added generic XML configuration stores and tests.
Decoupled configuration models from storage mechanism.
Internalised ILMerge dependencies to avoid issues referencing both project and dependencies in tests.
Got test project working with Appveyor.
Added better error handling if starting screensaver with invalid MAME executable path.
Added friendlier error if loading game list with invalid MAME executable path.

Notes

Configuration store

I decided to move away from the registry for configuration for three reasons:

Separating the configuration models from the configuration storage adds more flexibility, both for testing and interacting with unsaved changes.
XML-based configuration is easier to test.
I had already shifted your existing XML code for the game list into a separate store (high cohesion), so using this for everything else was very easy.

Dependency injection

As more and more classes are being added to the project, I thought it was about time to introduce DI. I went with Simple Injector as it is very fast, well used and straightforward to configure.

Using dependency injection means that management of class dependencies is both immediately visible and interactions far more consistent - no more new()ing services or using static classes which aren't always particularly easy to test.

I'm using constructor-based injection and classes are registered in its container automatically, so there's no real ceremony required to get it working.

As per standard DI practice, the only glue for Simple Injector is in Program - everything else is DI agnostic.

Code style

I'm using ReSharper's suggested code style for variable and method naming and suggested code simplifications. I haven't applied this across the entire solution, however as I've been working in areas I've been following its guidance (private fields have underscore prefix, prefer var, etc.).

Breaking changes

The breaking change in this PR is that, due to the configuration source changes to read from disk instead of the registry, previous MAME configuration is lost. I have added a user-friendly dialog if this is the case, so it should be fairly obvious to users.

Think that's all...

mika76 commented 5 years ago

Wow this looks and works great! You have done an amazing job. 💯 👍 🥇

Skip game validation (for custom MAME builds) What does this do? How does it work?

mika76 commented 5 years ago

Breaking changes

I think it's worth moving this to version 2 anyway - so many new features I think deserve a new version number...

nullpainter commented 5 years ago

Wow this looks and works great! You have done an amazing job. 💯 👍 🥇

Thank you! Sorry it's such a large PR - it kinda snowballed 😄

Skip game validation (for custom MAME builds) What does this do? How does it work?

This is a flag which skips the status check in GameListBuilder:

// Skip games which aren't fully emulated, unless check disabled in configuration
if (!_advancedSettings.SkipGameValidation)
{
  var status = driver.Attribute("status")?.Value;
  if (status != "good") continue;
 }

It's for people like me who have patched versions of MAME with the mandatory nag disabled - it's essentially a "I know what I'm doing" switch. TBH, I don't know how many people do have patched MAME builds, so I'm not going to die in a ditch if you think it's a superfluous feature.

nullpainter commented 5 years ago

I think it's worth moving this to version 2 anyway - so many new features I think deserve a new version number...

Ha, yes. I was going to say but forgot - I didn't update the version number in README.md because I wasn't sure what your versioning scheme was.

While we're on the subject, the pendant in me wonders if Mamesaver should at the very least be MameSaver - or, to be even more proper - MAMEsaver. What are your thoughts?

mika76 commented 5 years ago

I'm not going to die in a ditch if you think it's a superfluous feature

Ok so basically it changes how the games are added to the list - ie if they include a nag screen or not (I assume "rom is good" means that there is no nag screen) - I ❤️ the feature!

I wonder though if "skip game validation" is not expressive enough to tell people what it's actually doing? Maybe "Exclude non-complete games with nag screens when rebuilding games list" or something like that? Is this on or off by default? If you ask me it should be off and let em include nag screens only if they choose to...

mika76 commented 5 years ago

While we're on the subject, the pendant in me wonders if Mamesaver should at the very least be MameSaver - or, to be even more proper - MAMEsaver. What are your thoughts?

Good question 😆 I remember thinking about this originally years ago and then thought when you say "Mamesaver" it's one, very cool, word 😉 that rolls off the tongue.

nullpainter commented 5 years ago

I wonder though if "skip game validation" is not expressive enough to tell people what it's actually doing? Maybe "Exclude non-complete games with nag screens when rebuilding games list" or something like that?

Yes, I was wondering that. Don't want to be too verbose though and to be fair, it is on the advanced tab. That being said, maybe "Only include good games"? Or, if 'good' is a little too vague, "Only include fully supported games"?

Is this on or off by default? If you ask me it should be off and let em include nag screens only if they choose to...

Absolutely off by default. You can test this by trying out the 'Reset to defaults' button 😉

As an aside, defaults are set in the constructors of the various settings objects. This means that resetting to defaults is just a matter of creating a new parent Settings instance.

For convenience and reduced verbosity, the higher level child setting groups such as LayoutSettings and AdvancedSettings are provided via Simple Injector so they can just be passed into the constructor of classes that need them. This wiring is done in ContainerFactory:

container.Register(() => container.GetInstance<GeneralSettingsStore>().Get(), Lifestyle.Singleton);
container.Register(() => container.GetInstance<GameListStore>().GetGameList, Lifestyle.Singleton);

container.Register(() => container.GetInstance<Settings>().LayoutSettings, Lifestyle.Singleton);
container.Register(() => container.GetInstance<Settings>().AdvancedSettings, Lifestyle.Singleton);

The first two lines here are what's responsible for automatically retrieving the Settings and GameList from their respective XML stores and providing them as singletons. The second two are registering the child settings objects from Settings.

nullpainter commented 5 years ago

Wow this looks and works great!

btw, I ran it for 15 hours last night with the game timer set to one minute and it was still running happily in the morning 😄

(my laptop fan sounded like a jet engine this morning, mind...)

nullpainter commented 5 years ago

I've just noticed a handful of NullReferenceException in my logs. Unfortunately I was running a release build so didn't have a stack trace. This doesn't seem to have any user impact and doesn't consistently happen, so let's just keep an eye on it.

Apart from the outstanding discussion about the wording of the validation skip thing, I think this PR is good to go (unless I find and squash the null reference bug first).

I also added a couple of commits earlier today with a few very small tweaks to the dialog, as per commit comments.

mika76 commented 5 years ago

Ok - merged 👍

mika76 commented 5 years ago

I think let's both run it for a bit with logging and see how it goes - if all is good I can up the version and update the releases...

mika76 / mamesaver