Code refactor, thoughts

Code refactor, thoughts.md

Some thoughts about code structure, as I read code, and find some things harder to follow than others.

Smart pointers make poor arguments to methods/function

TL;DR Don't pass smart pointers are function arguments unless the function is meant to manage the lifetime of the data.

You use std::unique_ptr or shared_ptr to hold the values of some of your Data - this describes how is handled the lifetime of Data, but nothing else.

If you pass const std::unique_ptr<Data>& as an argument to a function/method, what are you describing ?

1. The holder type is const, so lifetime won't be affected
2. You tie the function behaviour to the lifetime specification, even if lifetime doesn't change
3. The pointed Data might be mutated (not const), but no easy way to tell
4. You need the reference to the holder to remain valid

All these bring no value, and IMHO only makes the argument type too long.

What should be preferred is passing arguments as [const] Data[*|&] :

1. The function has no need to know how the Data lifetime is handled
2. You can describe if const or nullptr apply

Booleans are values

(m_ui.SomeCheckBox->checkState() == Qt::CheckState::Checked) ? true : false

Should probably just be

(m_ui.SomeCheckBox->checkState() == Qt::CheckState::Checked)

Also

if (canRemoveItem)
{
  m_ui.RemoveBtn->setEnabled(true);
}
else
{
  m_ui.RemoveBtn->setEnabled(false);
}
m_ui.EditBtn->setEnabled(true);
m_ui.DuplicateBtn->setEnabled(true);

Should be

m_ui.RemoveBtn->setEnabled(canRemoveItem);
m_ui.EditBtn->setEnabled(true);
m_ui.DuplicateBtn->setEnabled(true);

Control flow, keep common cases together

if (canRemoveItem)
{
  m_ui.RemoveBtn->setEnabled(true);
  m_ui.EditBtn->setEnabled(true);
  m_ui.DuplicateBtn->setEnabled(true);
}
else
{
  m_ui.RemoveBtn->setEnabled(false);
  m_ui.EditBtn->setEnabled(true);
  m_ui.DuplicateBtn->setEnabled(true);
}

Should be

if (canRemoveItem)
{
  m_ui.RemoveBtn->setEnabled(true);
}
else
{
  m_ui.RemoveBtn->setEnabled(false);
}
m_ui.EditBtn->setEnabled(true);
m_ui.DuplicateBtn->setEnabled(true);

Constants should have names

// From the most significant bit to the least : coordinates(256), ..., name(8), identifier(4), ..., index(1)
// Mask value for index and identifier and name is 13 (8+4+1)
uint32_t displayObjectsMask = 13;

Should probably be

const uint32_t BIT_MASK_COORDINATES = (1u<<8);
...
const uint32_t BIT_MASK_NAME        = (1u<<3);
const uint32_t BIT_MASK_IDENTIFIER  = (1u<<2);
const uint32_t BIT_MASK_INDEX       = (1u<<0);
uint32_t displayObjectsMask = BIT_MASK_NAME | BIT_MASK_IDENTIFIER | BIT_MASK_INDEX;