From c108986639334be4f524964a5b5f377b6dab4e17 Mon Sep 17 00:00:00 2001 From: Nathan Woods Date: Sun, 16 Apr 2017 13:08:57 -0400 Subject: [PATCH] More options refactoring This should address outstanding concerns with PR#2231. I'm trying to turn emu_options into a self contained structure that encapsulates behaviors related to options, including the gymnastics pertaining to image/slot loading and interactions with get_default_card_software() and "just works". When the MAME 0.186 development cycle starts up, I hope to take this further. I want to make core_options::entry an abstract base class so that the entries associated with image options and slot options can derive from it. This will eliminate the current need for emu_options to directly expose maps for image and slot options. For now, I'm in stabilization mode, and I hope to get things working for a stable 0.185 release. --- src/emu/diimage.h | 1 + src/emu/emuopts.cpp | 126 +++++++++++++++++++++++++++------ src/emu/emuopts.h | 54 +++++++------- src/emu/image.cpp | 30 ++++---- src/emu/mconfig.cpp | 6 +- src/frontend/mame/mameopts.cpp | 21 ++---- src/lib/util/options.cpp | 18 ++++- src/lib/util/options.h | 11 ++- 8 files changed, 183 insertions(+), 84 deletions(-) diff --git a/src/emu/diimage.h b/src/emu/diimage.h index c13a70a61ac..23c22479217 100644 --- a/src/emu/diimage.h +++ b/src/emu/diimage.h @@ -250,6 +250,7 @@ public: } bool user_loadable() const { return m_user_loadable; } + const std::string &full_software_name() const { return m_full_software_name; } protected: // interface-level overrides diff --git a/src/emu/emuopts.cpp b/src/emu/emuopts.cpp index 00065030373..ada1c4f9ede 100644 --- a/src/emu/emuopts.cpp +++ b/src/emu/emuopts.cpp @@ -272,23 +272,28 @@ void emu_options::value_changed(const std::string &name, const std::string &valu // out image/slot options //------------------------------------------------- -bool emu_options::override_get_value(const char *name, std::string &value) const +core_options::override_get_value_result emu_options::override_get_value(const char *name, std::string &value) const { - auto slotiter = m_slot_options.find(name); - if (slotiter != m_slot_options.end() && slotiter->second.is_selectable()) + if (name) { - value = slotiter->second.value(); - return true; + auto slotiter = m_slot_options.find(name); + if (slotiter != m_slot_options.end()) + { + value = slotiter->second.specified_value(); + return slotiter->second.specified() + ? override_get_value_result::OVERRIDE + : override_get_value_result::SKIP; + } + + auto imageiter = m_image_options.find(name); + if (imageiter != m_image_options.end()) + { + value = imageiter->second; + return override_get_value_result::OVERRIDE; + } } - auto imageiter = m_image_options.find(name); - if (imageiter != m_image_options.end()) - { - value = imageiter->second; - return true; - } - - return false; + return override_get_value_result::NONE; } @@ -303,7 +308,7 @@ bool emu_options::override_set_value(const char *name, const std::string &value) auto slotiter = m_slot_options.find(name); if (slotiter != m_slot_options.end()) { - slotiter->second = parse_slot_option(std::string(value), true); + slotiter->second.specify(std::string(value)); return true; } @@ -319,18 +324,97 @@ bool emu_options::override_set_value(const char *name, const std::string &value) //------------------------------------------------- -// parse_slot_option - parses a slot option (the -// ',bios=XYZ' syntax) +// slot_option ctor //------------------------------------------------- -slot_option emu_options::parse_slot_option(std::string &&text, bool selectable) +slot_option::slot_option(const char *default_value) + : m_specified(false) + , m_default_value(default_value ? default_value : "") { - slot_option result; +} + + +//------------------------------------------------- +// slot_option::value +//------------------------------------------------- + +const std::string &slot_option::value() const +{ + // There are a number of ways that the value can be determined; there + // is a specific order of precedence: + // + // 1. Highest priority is whatever may have been specified by the user (whether it + // was specified at the command line, an INI file, or in the UI). We keep track + // of whether these values were specified this way + // + // Take note that slots have a notion of being "selectable". Slots that are not + // marked as selectable cannot be specified with this technique + // + // 2. Next highest is what is returned from get_default_card_software() + // + // 3. Last in priority is what was specified as the slot default. This comes from + // device setup + if (m_specified) + return m_specified_value; + else if (!m_default_card_software.empty()) + return m_default_card_software; + else + return m_default_value; +} + + +//------------------------------------------------- +// slot_option::specified_value +//------------------------------------------------- + +std::string slot_option::specified_value() const +{ + std::string result; + if (m_specified) + { + result = m_specified_bios.empty() + ? m_specified_value + : util::string_format("%s,bios=%s", m_specified_value, m_specified_bios); + } + return result; +} + + +//------------------------------------------------- +// slot_option::specify +//------------------------------------------------- + +void slot_option::specify(std::string &&text) +{ + // we need to do some elementary parsing here const char *bios_arg = ",bios="; size_t pos = text.find(bios_arg); - return pos != std::string::npos - ? slot_option(text.substr(0, pos), text.substr(pos + strlen(bios_arg)), selectable) - : slot_option(std::move(text), "", selectable); + if (pos != std::string::npos) + { + m_specified = true; + m_specified_value = text.substr(0, pos); + m_specified_bios = text.substr(pos + strlen(bios_arg)); + } + else + { + m_specified = true; + m_specified_value = std::move(text); + m_specified_bios = ""; + } } + +//------------------------------------------------- +// slot_option::set_bios +//------------------------------------------------- + +void slot_option::set_bios(std::string &&text) +{ + if (!m_specified) + { + m_specified = true; + m_specified_value = value(); + } + m_specified_bios = std::move(text); +} diff --git a/src/emu/emuopts.h b/src/emu/emuopts.h index 0de3d158ff2..2324783a09d 100644 --- a/src/emu/emuopts.h +++ b/src/emu/emuopts.h @@ -198,44 +198,48 @@ class slot_option { public: - slot_option() - : slot_option("", "", true) - { - } - - slot_option(std::string &&value, std::string &&bios, bool selectable) - : m_value(std::move(value)), m_bios(std::move(bios)), m_selectable(selectable) - { - } + slot_option(const char *default_value = nullptr); slot_option(const slot_option &that) = default; slot_option(slot_option &&that) = default; const slot_option &operator=(const slot_option &that) { - // I thought you got this implicitly by declaring a default copy constructor? - m_value = that.m_value; + m_specified = that.m_specified; + m_specified_value = that.m_specified_value; + m_specified_bios = that.m_specified_bios; m_default_card_software = that.m_default_card_software; - m_bios = that.m_bios; - return that; + m_default_value = that.m_default_value; + return *this; + } + + const slot_option &operator=(slot_option &&that) + { + m_specified = that.m_specified; + m_specified_value = std::move(that.m_specified_value); + m_specified_bios = std::move(that.m_specified_bios); + m_default_card_software = std::move(that.m_default_card_software); + m_default_value = std::move(that.m_default_value); + return *this; } // accessors - const std::string &value() const { return m_value.empty() ? m_default_card_software : m_value; } - const std::string &bios() const { return m_bios; } + const std::string &value() const; + std::string specified_value() const; + const std::string &bios() const { return m_specified_bios; } const std::string &default_card_software() const { return m_default_card_software; } - bool is_default() const { return m_value.empty(); } - bool is_selectable() const { return m_selectable; } + bool specified() const { return m_specified; } // seters - void set_bios(std::string &&s) { m_bios = std::move(s); } + void specify(std::string &&text); + void set_bios(std::string &&text); void set_default_card_software(std::string &&s) { m_default_card_software = std::move(s); } - void set_is_selectable(bool selectable) { m_selectable = selectable; } private: - std::string m_value; - std::string m_bios; - std::string m_default_card_software; - bool m_selectable; + bool m_specified; + std::string m_specified_value; + std::string m_specified_bios; + std::string m_default_card_software; + std::string m_default_value; }; @@ -435,11 +439,9 @@ public: std::map &image_options() { return m_image_options; } const std::map &image_options() const { return m_image_options; } - static slot_option parse_slot_option(std::string &&text, bool selectable); - protected: virtual void value_changed(const std::string &name, const std::string &value) override; - virtual bool override_get_value(const char *name, std::string &value) const override; + virtual override_get_value_result override_get_value(const char *name, std::string &value) const override; virtual bool override_set_value(const char *name, const std::string &value) override; private: diff --git a/src/emu/image.cpp b/src/emu/image.cpp index 0a8f0d33fb8..a8548fca75d 100644 --- a/src/emu/image.cpp +++ b/src/emu/image.cpp @@ -38,10 +38,11 @@ image_manager::image_manager(running_machine &machine) continue; // is an image specified for this image? - if (machine.options().image_options().count(image.instance_name()) > 0) + auto iter = machine.options().image_options().find(image.instance_name()); + if (iter != machine.options().image_options().end() && !iter->second.empty()) { // we do have a startup image specified - load it - const std::string &startup_image = machine.options().image_options()[image.instance_name()]; + const std::string &startup_image(iter->second); image_init_result result = image_init_result::FAIL; // try as a softlist @@ -174,24 +175,23 @@ int image_manager::write_config(emu_options &options, const char *filename, cons void image_manager::options_extract() { - /* only extract the device options if we've added them - no need to assert in case they are missing */ + for (device_image_interface &image : image_interface_iterator(machine().root_device())) { - int index = 0; - - for (device_image_interface &image : image_interface_iterator(machine().root_device())) + // we have to assemble the image option differently for software lists and for normal images + std::string image_opt; + if (image.exists()) { - const char *filename = image.filename(); - - /* and set the option */ - std::string error; - machine().options().set_value(image.instance_name().c_str(), filename ? filename : "", OPTION_PRIORITY_CMDLINE, error); - - index++; + if (image.loaded_through_softlist()) + image_opt = util::string_format("%s:%s:%s", image.software_list_name(), image.full_software_name(), image.brief_instance_name()); + else + image_opt = image.filename(); } + + // and set the option + machine().options().image_options()[image.instance_name()] = std::move(image_opt); } - /* write the config, if appropriate */ + // write the config, if appropriate if (machine().options().write_config()) write_config(machine().options(), nullptr, &machine().system()); } diff --git a/src/emu/mconfig.cpp b/src/emu/mconfig.cpp index 841580a4f08..84fc5c70b67 100644 --- a/src/emu/mconfig.cpp +++ b/src/emu/mconfig.cpp @@ -44,6 +44,10 @@ machine_config::machine_config(const game_driver &gamedrv, emu_options &options) bool is_default; if (!has_option) { + // Theoretically we should never get here; in the long run the expectation is that + // options.slot_options() should be fully qualified and all options should be + // present. However, we're getting late in the MAME 0.185 development cycle and + // I don't want to rip this out (yet) selval = slot.default_option(); is_default = true; } @@ -51,7 +55,7 @@ machine_config::machine_config(const game_driver &gamedrv, emu_options &options) { const slot_option &opt = options.slot_options()[slot_option_name]; selval = opt.value().c_str(); - is_default = opt.is_default(); + is_default = !opt.specified(); } if (selval && *selval) diff --git a/src/frontend/mame/mameopts.cpp b/src/frontend/mame/mameopts.cpp index f610fbdf8ac..54609a601a1 100644 --- a/src/frontend/mame/mameopts.cpp +++ b/src/frontend/mame/mameopts.cpp @@ -57,20 +57,14 @@ bool mame_options::add_slot_options(emu_options &options, value_specifier_func v // add the option options.add_entry(name, nullptr, OPTION_STRING | OPTION_FLAG_DEVICE, slot.default_option(), true); - options.slot_options()[name] = slot_option(); + options.slot_options()[name] = slot_option(slot.default_option()); // allow opportunity to specify this value if (value_specifier) { - std::string value = value_specifier(name); - if (value != value_specifier_invalid_value()) - { - const device_slot_option *option = slot.option(value.c_str()); - if (option) - { - options.slot_options()[name] = emu_options::parse_slot_option(std::move(value), option->selectable()); - } - } + std::string specified_value = value_specifier(name); + if (specified_value != value_specifier_invalid_value()) + options.slot_options()[name].specify(std::move(specified_value)); } } } @@ -307,13 +301,6 @@ bool mame_options::reevaluate_slot_options(emu_options &options) if (options.slot_options()[name].default_card_software() != default_card_software) { options.slot_options()[name].set_default_card_software(std::move(default_card_software)); - - // the value of this slot option has changed. we may need to change is_selectable(); this - // should really be encapsulated within emu_options - const device_slot_option *option = slot.option(options.slot_options()[name].value().c_str()); - if (option) - options.slot_options()[name].set_is_selectable(option->selectable()); - result = true; } } diff --git a/src/lib/util/options.cpp b/src/lib/util/options.cpp index 16b62f9c91f..b835d0fa681 100644 --- a/src/lib/util/options.cpp +++ b/src/lib/util/options.cpp @@ -565,9 +565,21 @@ std::string core_options::output_ini(const core_options *diff) const for (entry &curentry : m_entrylist) { const char *name = curentry.name(); - const char *value = name && override_get_value(name, overridden_value) - ? overridden_value.c_str() - : curentry.value(); + const char *value; + switch (override_get_value(name, overridden_value)) + { + case override_get_value_result::NONE: + default: + value = curentry.value(); + break; + + case override_get_value_result::SKIP: + continue; + + case override_get_value_result::OVERRIDE: + value = overridden_value.c_str(); + break; + } bool is_unadorned = false; // check if it's unadorned diff --git a/src/lib/util/options.h b/src/lib/util/options.h index b2bc46343ac..637481b12b2 100644 --- a/src/lib/util/options.h +++ b/src/lib/util/options.h @@ -181,8 +181,17 @@ public: int options_count() const { return m_entrylist.count(); } protected: + // This is a hook to allow option value retrieval to be overridden for various reasons; this is a crude + // extensibility mechanism that should really be replaced by something better + enum class override_get_value_result + { + NONE, + OVERRIDE, + SKIP + }; + virtual void value_changed(const std::string &name, const std::string &value) {} - virtual bool override_get_value(const char *name, std::string &value) const { return false; } + virtual override_get_value_result override_get_value(const char *name, std::string &value) const { return override_get_value_result::NONE; } virtual bool override_set_value(const char *name, const std::string &value) { return false; } private: