This change demonstrates how to deal properly with possible duplicate entities
with similar symbolic ID: define a RandomID (to guarantee a distinct hash on each instance).
In the actual implementation, this should happen already within the domain model,
not when constructing the diff (obviously of course...)
This change also adds a mutation sequence to inject the actual track name
so this seems to be the better approach for dealing with this insidious problem.
In some cases -- as here most prominently with the root track within the timeline --
we have to care within the domain model to prepare unique ids even for sub objects
treated as attributes. In the actual case, without that special attention,
all timelines would hold onto an attribute "fork" with the same ID, based
on the type of the nested object plus the string "fork". Thus all root track
representations in the GUI would end up listening to the same ID on the UI-Bus...
...rather extend the "object builder" DSL notation to allow passing in a given EntryID literally.
Rationale is, we should handle the problem of unique IDs on the level of the domain model.
If we attempt to "fix" this within GenNode, the price would be to make the ETD creation stateful
this is not a problem, strictly sepaking, locally.
But it becomes a problem once the GUI uses those attribute IDs
as Element-IDs for tangible UI entities, which need to be uniquely
addressable via the UI-Bus.
An obvious solution is to inject randomness into the Attribute ID hash
other than the regular way of building an object,
we do expect a minimal structure to be sent right within the INS message.
Rationale: the standard way would allow for too much leeway and created
unwanted intermediary states. The non-standard way decided upon here
is well within the limits of our diff language
As it turns out, using the functional-notation form conversion
with *parentheses* will fall back on a C-style (wild, re-interpret) cast
when the target type is *not* a class. As in the case in question here, where
it is a const& to a class. To the contrary, using *curly braces* will always
attempt to go through a constructor, and thus fail as expected, when there is
no conversion path available.
I wasn't aware of that pitfall. I noticed it since the recently introduced
class TimelineGui lacked a conversion operator to BareEntryID const& and just
happily used the TimelineGui object itself and did a reinterpret_cast into BareEntryID
...these magical strings are already spreading dangerously throughout the code base
PS: also fixup for c6b8811af0 (broken whitespace in test definition)
Considering the fact that we are bound to introduce yet another iteration control function,
because there is literally no other way to cause a refresh within the IterTreeExplorer-Layers,
it is indicated to reconsider the way how IterStateWrapper attaches to the
iteration control API.
As it turns out, we'll never need an ADL-free function here;
and it seems fully adequate to require all "state core" objects to expose
the API as argument less member function. Because these reflect precisely
the contract of a "state core", so why not have them as member functions.
And as a nice extra, the implementation becomes way more concise in
all the cases refactored with this changeset!
Yet still, we stick to the basic design, *not* relying on virtual functions.
So this is a typical example of a Type Class (or "Concept" in C++ terminology)
- always layer the TreeExplorer (builder) on top of the stack
- always intersperse an IterableDecorator in between adjacent layers
- consequently...
* each layer implementation is now a "state core"
* and the source is now always a Lumiera Iterator
This greatly simplifies all the type rebindings and avoids the
ambiguities in argument converison. Basically now we can always convert
down, and we just need to pick the result type of the bound functor.
Downside is we have now always an adaptation wrapper in between,
but we can assume the compiler is able to optimise such inline
accessors away without overhead.
up to now, we allowed only initialisation with a precisely matching type.
But this special case seems worth supporting, since it typically occurs
within the "object builder" syntax based on Rec::Mutator
Explicitly assuming that those functions are called solely from IterAdapter
and that they are implemented in a typical standard style, we're able to elide
two redundant calls to the checkPoint() function. Since checkPoint typically performs
some non-trivial checks, this has the potential of a significant performance improvement
- we check (and throw ITER_EXHAUST) anyway from operator++, so we know that pos is valid
- the iterate() function ensures checkPoint is invoked right after iterNext,
and thus the typical standard implementation of iterNext need not do the same
basically DiffMessage has a "take everything" ctor, which happens
to match on type DiffMessage itslef, since the latter is obviously
a Lumiera Forward Operator. Unfortunately the compiler now considers
this "take everyting" ctor as copy constructor. Worse even, such a
template generated ctor qualifies as "best match".
The result was, when just returing a DiffMessage by value form a
function, this erroneous "copy" operation was invoked, thus wrapping
the existing implementation into a WrappedLumieraIterator.
The only tangible symptom of this unwanted storage bloat was the fact
that our already materialised diagnostics where seemingly "gone". Indee
they weren't gone for real, just covered up under yet another layer of
DiffMessage wrapping another Lumiera Forward Iterator
this becomes more relevant now, since the actual MutationMessage iterators
are implemented in terms of a shared_ptr to IterSource. Thus, when building
processing pipelines, we most definitively want to move that smart-ptr into
the destination, since this avoids touching the shared count and thus avoids
generating unnecessary memory barriers.
now this highlights the unsettled decision still the more,
as can be seen by all that unnecessary copying. Basically we move the
Diff into the lambda-closure, from there into an anonymous instance,
from there into the embedded Buffer in MutationMessage, which again
just happens to sit in the closure storage when the action is invoked.
And all of this copying just to move the DiffMessage for consumption
into the TreeMutator...
thus by #1066 we should really get rid of the MutationMessage class altogether!
actually I do not know much regarding the actual situation when,
within the Builder run, we're able to detect a change and generate
a diff description. However, as a first step, I'll pick IterSrouce
as a base interface and use a "generation context", which is to be
passed by shared-ptr
This changeset fixes a huge pile of problems, as indicated in the
error log of the Doxygen run after merging all the recent Doxygen improvements
unfortunately, auto-linking does still not work at various places.
There is no clear indication what might be the problem.
Possibly the rather unstable Sqlite support in this Doxygen version
is the cause. Anyway, needs to be investigated further.
...the sheer amount of mechanical replacements scattered all over these
files might be a vivid indication, that the design of the interface system
is subobptimal ;-)
Phew, convoluted.
And I was doubtful that we need to support multiple typed child collection
Well, we get three such collections already in the first real world example...
Damn sideeffect of the suppport for move-only types: since we're
moving our binding now into place /after/ construction, in some cases
the end() iterator (embedded in RangeIter) becomes invalid. Indeed this
was always broken, but didn't hurt, as long as we only used vectors.
Solution: use a dedicated init() hook, which needs to be invoked
*after* the TreeMutator has been constructed and moved into the final
location in the stack buffer.
unintentionally we used copy construction in the builder expression,
wenn passing in the CollectionBinding to the ChildCollectionMutator.
The problem is that CollectionBinding owns a shaddow buffer, where
the contents of the target collection are moved temporarily while
applying the diff. The standard implementation of copy construction
would cause a copy of that shaddow buffer, which boils down to
a copy of the storage of the target collection.
If we want to support move-only types in the collection, most notably
std::unique_ptr, we can thus only use the move constructor. Beyond that
there is no problem, since we're only ever moving elements, and new
elements will be move constructed via emplace() or emplace_back()
actually this is a pragmatic extension for some special use cases,
and in general rather discurraged, since it contradicts the
established diff semantics. Yet with some precaution, it should
be possible to transport information via an intermediary ETD
Map -> ETD -> Map
for the record: while it is indeed sweet-and-simple to support Ref::THIS
here, it is near impossible to represent it in general, in a setup with
multiple "onion-layers". The reason is, we'd have to incorporate such
special treatment into the /selector predicate/, which in turn undermines
the ability to pick the right onion layer to handle a given diff verb,
since "Ref::THIS" is a generic marker and we have no other data to base
the decision in the selector on.
yay! this piece of code has served its purpose:
it was the blueprint to build a way better design and implementation,
which can now cover this "generic tree" case as a special case as well
this adds kind of an extension point to diff::Record<GenNode>::Mutator,
which is then actually defined (implemented) within the diff framework.
This allows the TreeDiffTraits automatically to use this function
to get a TreeMutator for a given Rec::Mutator. Which in turn allows
the generic version of DiffApplicator automatically to attach and
bind to a Record<GenNode>
together this allows us to ditch the explicit specialisation
and dedicated, hand-written implementation of DiffApplication
to GenNode in favour of using the TreeMutator and friends.
