Proposed smtk::attribute::GroupItem::shiftGroup() method

[johnt]

FYI all, I plan to add a public method to GroupItem to support reordering of subgroups. The target use case is for a new custom view for Truchas induction coils; specifically we want to allow dragging and dropping rows in a table that displays time steps vs frequency and electrical current data. The planned signature is

// smtk::attribute::GroupItem::
bool shiftGroup(std::size_t fromIndex, std::size_t toIndex);
// fromIndex: position of the selected subgroup to be moved
// toIndex: position of the subgroup after the move is complete
// return value indicates whether move was completed or not

It is a relatively minor change, but because I don’t touch smtkCore’s guts very often, I thought I should give everyone a chance to comment beforehand.

Related to the same use case, there is also a need to “sort” the table based on the time values. My current thinking is for the custom view code to figure out the sorted order, and call the shiftGroup() method, possibly multiple times, to update the GroupItem.

[johnt]

Considering some alternative names, in current order of preference

    bool moveGroup(std::size_t fromPosition, std::size_t toPosition)
    bool repositionGroup(...)
    bool shiftGroup(...)
    bool relocateGroup(...)

[johnt]

Side note, I plan to add the equivalent functionality to ValueItem/ValueItemTemplate.

[dcthomp]

@johnt If you are going to provide the same functionality in other classes, how about removing Group from the name so it can be shared across classes? I like move() better than the others. Especially shift() which to me implies stack semantics like rot, swap, over in Forth (because my old HP calculator did things that way).

If the API were a little more generic, permute() or reorder() might make sense. But it sounds like those are out of scope.

[johnt]

Are you suggesting a templated function to move items within an std::vector? (I’m a bit surprised there isn’t one in std::algorithm.) Maybe I should add a VectorUtil.h file to smtk/common for this, analogous, in some ways, to the StringUtil class.

template <typename T>
bool smtk::common::VectorUtil::move(
  std::vector<T>& v, std::size_t fromPosition, std::size_t toPosition)

---

As for smtk::attribute, to add a little more detail, my current thinking is to add two functions:

bool smtk::attribute::GroupItem::moveGroup(std::size_t fromPosition, std::size_t toPosition)
bool smtk::attribute::ValueItemTemplate::moveValue(std::size_t fromPosition, std::size_t toPosition)

The names are consistent with the current API methods:

• GroupItem’s appendGroup() and removeGroup()
• ValueItemTemplate’s appendValue() and removeValue()

As an aside, I don’t foresee the internal logic being reused between the group and value methods, because the value item method needs to consider discrete-value and expression options. But both methods should be able to use a common VectorUtil::move() method.

[dcthomp]

@johnt

Are you suggesting a templated function to move items within an std::vector? (I’m a bit surprised there isn’t one in std::algorithm.)

No, there is already a move function in the <algorithm> header [1].

@johnt

As for smtk::attribute, to add a little more detail, my current thinking is to add two functions: …

What I was suggesting is that these functions share the same name; there is no need to add part of the class name to the end of the method — the name and type of the variable usually make it quite apparent what you are doing. If you must name the methods moveXXX() instead of just move(), I would advocate for moveEntry() or something generic that can be shared across both classes. This makes it possible for the class to be a template parameter to a function that moves things without caring what the container contains. If SMTK insists on class-unique method names for identical functionality then we cannot use classes as templates, which is a bummer.

[tj.corona]

Are you suggesting a templated function to move items within an std::vector? (I’m a bit surprised there isn’t one in std::algorithm.)

No, there is already a move function in the <algorithm> header [1 ].

+1. We should be using the STL for this

there is no need to add part of the class name to the end of the method — the name and type of the variable usually make it quite apparent what you are doing.

+1 for a terse API.

If SMTK insists on class-unique method names for identical functionality then we cannot use classes as templates, which is a bummer.

Agreed, total bummer. There are ways around it, but since we control both sides of the API it is better to standardize the API now then to code around awkward API later.

[tj.corona]

@dcthomp

What I was suggesting is that these functions share the same name; there is no need to add part of the class name to the end of the method — the name and type of the variable usually make it quite apparent what you are doing.

Time notwithstanding, the STL operations for move, copy, find, accumulate, etc. all use a generic iterator-based interface. We have containers that can be iterated; perhaps the better play is to expose iterators for our containers. They can be (but need not be) simply iterators to the underlying container; that would be < 20 lines of code across the entire attribute system.

[dcthomp]

@tj.corona

We have containers that can be iterated; perhaps the better play is to expose iterators for our containers.

I have done this in the past (and there are begin() and end() methods in ValueItemTemplate thanks to YT), but it does mean that now the container class doesn’t know when its internal storage has been changed. This is especially problematic when things like isSet(int i) rely on a different container having the same length and ordering as the one you just borked by calling std::move(...) on the values.

So, I think read access via iterators is great, but write access needs to be considered very carefully and either avoided or guarded or at least really well-documented.

[tj.corona]

@dcthomp

the container class doesn’t know when its internal storage has been changed. This is especially problematic when things like isSet(int i) rely on a different container having the same length and ordering as the one you just borked by calling std::move(...) on the values.

@tj.corona

They can be (but need not be) simply iterators to the underlying container

Noted: change that to “cannot be simply iterators to the underlying container.”

[johnt]

Hmm, if I understand correctly, std::move() doesn’t shift the data, as would be needed for a drag & drop operation.

As an example, for a vector of integers

[0, 1, 2, 3, 4, 5]

I believe that a std::move call from positions [4,5) to 2 would change the vector to

[0, 1, 4, 3, undefined, 5]

whereas the desired result is

[0, 1, 4, 2, 3, 5]

Perhaps, as you suggest, I need a better name for this function?

• reorder() as you proposed
• moveAndShift()
• shuffleOne()

[johnt]

Oh, never mind, looks like std::rotate() is what we need…

[dcthomp]

@johnt

Oh, never mind, looks like std::rotate() is what we need…

Sounds like a good method name. It will be what people familiar w/ the STL would expect.

[tj.corona]

@johnt

Oh, never mind, looks like std::rotate() is what we need…

Or insert. Since vectors are guaranteed to be contiguous in memory, inserting and deleting the original range could be analogous to rotate.

EDIT: ah, never mind. I misunderstood the desired effect.

[johnt]

Thanks for the thoughtful discussions. With the hope of making this my last post to this topic , here is where I am at this morning:

1. Plan to add two methods:

bool smtk::attribute::GroupItem::rotate(std::size_t fromPosition, std::size_t toPosition)
bool smtk::attribute::ValueItemTemplate::rotate(std::size_t fromPosition, std::size_t toPosition)

2. The return value is true if the underlying data is rotated (even if the end result looks the same, for example, if all value items have the same value). This implies that the return value is false if:

• either position argument is outside the range of the item’s underlying data
• the two position arguments are equal to each other

3. I have no plans to add a generic rotate_any_smtk_object() function; if such a function is potentially useful, then some other project will have to implement it.

4. But if we think rotating data might be applicable to other attribute item types, I am open to adding a virtual method in the base class, that is:

virtual bool smtk::attribute::Item::rotate(std::size_t fromPosition, std::size_t toPosition)
{
  (void)fromPosition;
  (void)toPosition;
  return false;
}