Proposal: enhance nomenclature for slots and string defines
Description
Services have billions of slots and string defines. As our standard requires them to have a prefix and / or a suffix, that really obfuscates the code.
Proposal
The proposal could be summed up by the following points:
- Use a
namespace
-likestruct
(see below) for slots / signals / configuration instead of a suffix. - Do not use the
s_
for `static constants.
Edit_FB: please have a look at the comments section below to review the latest proposal.
Examples - Initial proposal
Old - Initial proposal
1 - Slots keys
Don't do
static const core::com::Slots::SlotKeyType YourService::s_SET_DOUBLE_PARAMETER_SLOT = "setDoubleParameter";
static const core::com::Slots::SlotKeyType YourService::s_SET_INT_PARAMETER_SLOT = "setIntParameter";
static const core::com::Slots::SlotKeyType YourService::s_SET_BOOL_PARAMETER_SLOT = "setBoolParameter";
static const core::com::Slots::SlotKeyType YourService::s_SET_STRING_PARAMETER_SLOT = "setStringParameter";
Do
class YourService : public blablabla
{
//...
protected:
struct slot_key
{
using type = core::com::Slots::SlotKeyType;
static inline const type SET_DOUBLE_PARAMETER = "setDoubleParameter";
static inline const type SET_INT_PARAMETER = "setIntParameter";
static inline const type SET_BOOL_PARAMETER = "setBoolParameter";
static inline const type SET_STRING_PARAMETER = "setStringParameter";
};
//...
};
2 - Configurations / objects
Don't do
static const std::string YourService::s_VISIBLE_CONFIG = "visible";
static const std::string YourService::s_MATERIAL_TEMPLATE_CONFIG = "materialTemplate";
static const std::string YourService::s_TEXTURE_NAME_CONFIG = "textureName";
static const std::string YourService::s_FILTERING_CONFIG = "filtering";
static const std::string YourService::s_SCALING_CONFIG = "scaling";
static const std::string YourService::s_RADIUS_CONFIG = "radius";
static const std::string YourService::s_DISPLAY_LABEL_CONFIG = "displayLabel";
static const std::string YourService::s_LABEL_COLOR_CONFIG = "labelColor";
static const std::string YourService::s_COLOR_CONFIG = "color";
static const std::string YourService::s_FIXED_SIZE_CONFIG = "fixedSize";
static const std::string YourService::s_QUERY_CONFIG = "queryFlags";
static const std::string YourService::s_FONT_SOURCE_CONFIG = "fontSource";
static const std::string YourService::s_FONT_SIZE_CONFIG = "fontSize";
Do
class YourService : public blablable
{
//...
protected:
struct config
{
// Note: I am not sure this is a good idea to have this in the same structure as the actual attributes updated at
// runtime. This is just a demo, feel free to update this proposal with better nomenclature.
static inline const std::string VISIBLE = "visible";
static inline const std::string MATERIAL_TEMPLATE = "materialTemplate";
static inline const std::string TEXTURE_NAME = "textureName";
static inline const std::string FILTERING = "filtering";
static inline const std::string SCALING = "scaling";
static inline const std::string RADIUS = "radius";
static inline const std::string DISPLAY_LABEL = "displayLabel";
static inline const std::string LABEL_COLOR = "labelColor";
static inline const std::string COLOR = "color";
static inline const std::string FIXED_SIZE = "fixedSize";
static inline const std::string QUERY = "queryFlags";
static inline const std::string FONT_SOURCE = "fontSource";
static inline const std::string FONT_SIZE = "fontSize";
bool visible = false;
std::string material = "...";
// ...
};
struct objects
{
struct inout
{
static inline const std::string mask = "...";
static inline const std::string tf = "...";
static inline const std::string volume = "...";
};
struct in
{
static inline const std::string matrix = "...";
};
};
config m_config = {};
};
3 - Signals
Don't do:
class YourService : public blablable
{
typedef core::com::Signal<void ()> BufferModifiedSignalType;
DATA_API static const core::com::Signals::SignalKeyType s_BUFFER_MODIFIED_SIG;
typedef core::com::Signal<void (SPTR(Point))> LandmarkAddedSignalType;
DATA_API static const core::com::Signals::SignalKeyType s_LANDMARK_ADDED_SIG;
};
Do:
class YourService : public blablable
{
struct signal
{
using buffer_modified = core::com::Signal<void()>;
using landmark_added = core::com::Signal<void(std::shared_ptr<Point>)>;
struct key
{
// See notes below as for why I am changing the case here
static inline const std::string buffer_modified = "...";
static inline const std::string landmark_added = "...";
};
};
};
Examples
SDoStuff::SDoStuff() noexcept
{
// Old
{
newSlot(s_LOAD_NEW_STUFF_SLOT, &SDVR::load_new_stuff, this);
newSlot(s_UPDATE_SOMETHING_SLOT, &SDVR::update_something, this);
newSlot(s_TOGGLE_WIDGET_SLOT, &SDVR::toggle_widget, this);
newSignal<compute_done_signal>(s_COMPUTE_SUCCESS_SIGNAL);
newSignal<compute_done_signal>(s_COMPUTE_FAILED_SIGNAL);
}
// New
{
newSlot(slot_key::LOAD_NEW_STUFF, &SDVR::load_new_stuff, this);
newSlot(slot_key::UPDATE_SOMETHING, &SDVR::update_something, this);
newSlot(slot_key::TOGGLE_WIDGET, &SDVR::toggle_widget, this);
newSignal<signal::compute_success>(signal::key::compute_success);
newSignal<signal::compute_failed>(signal::key::compute_failed);
}
}
//-----------------------------------------------------------------------------
sight::service::IService::KeyConnectionsMap SDoStuff::getAutoConnections() const
{
return
{
{objects::inout::mask, data::Image::signal::key::modified, slot_key::make_money},
{objects::inout::tf, data::TransferFunction::signal::key::modified, slot_key::print_dollars},
{objects::inout::volume, data::Image::signal::key::modified, slot_key::sell_purridge},
};
}
//-----------------------------------------------------------------------------
void SDoStuff::configuring()
{
// ...
const auto xml_config = this->getConfiguration().get_child("config.<xmlattr>");
{
m_config.visible = xml_config.get<bool>(config::VISIBLE, true);
m_config.material = xml_config.get<std::strinh>(config::MATERIAL_TEMPLATE, "");
}
// ...
}
Examples - Updated version
Suggested by @fbridault in the comments section below. For the details on how we got there, please unfold the section above.
class YourService : public blablable
{
//...
protected:
struct slot
{
using type = core::com::slots::key_t;
static inline const type SET_DOUBLE_PARAMETER = "setDoubleParameter";
static inline const type SET_INT_PARAMETER = "setIntParameter";
// Just to say that the type alias is not mandatory, imho
static inline const core::com::slots::key_t SET_BOOL_PARAMETER = "setBoolParameter";
// Or just use an alias that would be defined in service::IService/service::base for instance
static inline const slot_key_t SET_STRING_PARAMETER = "setStringParameter";
};
// Awesome, but this section is only mandatory IMHO if the keys are used more than once
// Otherwise, the strings could be put directly in the configuring() method
struct config
{
static inline const std::string VISIBLE = "visible";
static inline const std::string MATERIAL_TEMPLATE = "materialTemplate";
static inline const std::string TEXTURE_NAME = "textureName";
static inline const std::string FILTERING = "filtering";
static inline const std::string SCALING = "scaling";
static inline const std::string RADIUS = "radius";
static inline const std::string DISPLAY_LABEL = "displayLabel";
static inline const std::string LABEL_COLOR = "labelColor";
static inline const std::string COLOR = "color";
static inline const std::string FIXED_SIZE = "fixedSize";
static inline const std::string QUERY = "queryFlags";
static inline const std::string FONT_SOURCE = "fontSource";
static inline const std::string FONT_SIZE = "fontSize";
// Nice suggestion indeed, this helps to isolate
bool visible = false;
std::string material = "...";
// ...
};
// Here I would skip the "objects" struct for the sake of simplicity
// I would keep the upper-case for consistency (but got rid of the s_ prefix, I think this is useless for const members)
struct inout
{
static inline const std::string MASK = "...";
static inline const std::string TF = "...";
static inline const std::string VOLUME = "...";
};
struct in
{
static inline const std::string MATRIX = "...";
};
struct signal
{
using buffer_modified_t = core::com::Signal<void()>;
using landmark_added_t = core::com::Signal<void(std::shared_ptr<Point>)>;
// If we stick with the upper-case, I think we could implicitly deduce that these are keys, which is a bit shorter
static inline const std::string BUFFER_MODIFIED = "...";
static inline const std::string LANDMARK_ADDED = "...";
};
config m_config = {};
};
Which would look like:
SDoStuff::SDoStuff() noexcept
{
newSlot(slot::LOAD_NEW_STUFF, &SDVR::load_new_stuff, this);
newSlot(slot::UPDATE_SOMETHING, &SDVR::update_something, this);
newSlot(slot::TOGGLE_WIDGET, &SDVR::toggle_widget, this);
newSignal<signal::compute_success>(signal::COMPUTE_SUCCESS);
newSignal<signal::compute_failed>(signal::COMPUTE_FAILED);
}
//-----------------------------------------------------------------------------
sight::service::IService::KeyConnectionsMap SDoStuff::getAutoConnections() const
{
return
{
{inout::mask, data::Image::signal::MODIFIED, slot::MAKE_MONEY},
{inout::tf, data::TransferFunction::signal::MODIFIED, slot::PRINT_DOLLARS},
{inout::volume, data::Image::signal::MODIFIED, slot::SELL_PURRIDGE},
};
}
Notes
- In theory should be possible to extend
newSignal
/newSlot
to deduce the key. BUT since we sometimes use the same signal types with different keys, this isn't going to work. - I am not sure if the capitalisation is a GoodThing™, thus the style change for signals illustrated above.
Outcomes
Well, I am not too sure how to describe how I feel about my proposal.
From a technical point of view, I think the best outcome is an enforcement of an XML-like architecture in the C++ code. I believe this would make services easier to read and to "link" to the XML configurations they are used in. And also, this makes less warnings related to non-POD statics (which we disable for this reason).
I however think that this is more of a "quality of life" improvement for... Developers. I believe that such a nomenclature would be easier to grasp for newcomers. And easier to manipulate for the current staff. I strongly believe that such improvements are in fact crucial: nobody wants to work within a framework which code is hard to decipher due to its nomenclature being inexpressive or hard to follow.
This obviously doesn't mean that this nomenclature is perfect and I believe this is still a bit too verbose. Feel free to edit the proposal or add one in the paragraph above.
Links / references
I tried to apply that here, but got a slap on the wrist. For very good reasons. So feel free to throw things at me.