The ClipPresenter can access the CanvasHook wired into its actual ClipDelegate (widget).
And this in turn exposes the DisplayMetric, with the ability to transform
presentation coordinates (pixels) into a model representation (Time)
The actual translation is still hardwired placeholder code,
since it is planned to build an generic component "ZoomWindow"
to provide all the typical zomming and view window translations
found in every timeline editor
- move construct into the buffer
- directly invoke the payload constructor through PlantingHandle
- reconsider type signature and size constraint
- extend the unit test
- document a corner case of c++ "perfect forwarding",
which caused me some grief here
...this extension was spurred by the previeous refactoring.
Since 'emplace' now clearly denotes an operation to move-embed an existing object,
we could as well offer a separate 'create' API, which would take forwarding
arguments as usual and just delegates to the placement-new operation 'create'
already available in the InPlaceBuffer class.
Such would be a convenience shortcut and is not strictly necessary,
since move-construction is typically optimised away; yet it would also
allow to support strictly non-copyable payload types.
This refactoring also highlights a fuzziness in the existing design,
where we just passed the interface type, while being sloppy about the
DEFAULT type. In fact this *is* relevant, since any kind of construction
might fail, necessitating to default-construct a placeholder, since
InPlaceBuffer was intended for zero-overhead usage and thus has in itself
no means to know about the state of its buffer's contents. Thus the
only sane contract is that there is always a valid object emplaced
into the buffer, which in turn forces us to provide a loophole for
class hierarchies with an abstract base class -- in such a case the
user has to provide a fallback type explicitly.
...for the operation on a PlantingHandle, which allows
to implant a sub type instance into the opaque buffer.
* "create" should be used for a constructor invocation
* "emplace" takes an existing object and move-constructs
...in an attempt to clarify why numerous cross links are not generated.
In the end, this attempt was not very successful, yet I could find some breadcrumbs...
- file comments generally seem to have a problem with auto link generation;
only fully qualified names seem to work reliably
- cross links to entities within a namespace do not work,
if the corresponding namespace is not documented in Doxygen
- documentation for entities within anonymous namespaces
must be explicitly enabled. Of course this makes only sense
for detailed documentation (but we do generate detailed
documentation here, including implementation notes)
- and the notorious problem: each file needs a valid @file comment
- the hierarchy of Markdown headings must be consistent within each
documentation section. This entails also to individual documented
entities. Basically, there must be a level-one heading (prefix "#"),
otherwise all headings will just disappear...
- sometimes the doc/devel/doxygen-warnings.txt gives further clues
...by relying on the newly implemented automatic standard binding
Looks like a significant improvement for me, now the actual bindings
only details aspects, which are related to the target, and no longer
such technicalitis like how to place a Child-Mutator into a buffer handle
After this long break during the "Covid Year 2020",
I pick this clean-up task as a means to fresh up my knowledge about the code base
The point to note is, when looking at all the existing diff bindings,
seemingly there is a lot of redundancy on some technical details,
which do not cary much meaining or relevance at the usage site:
- the most prominent case is binding to a collection of DiffMutables hold by smart-ptr
- all these objects expose an object identity (getID() function), which can be used as »Matcher«
- and all these objects can just delegate to the child's buildMutator() function
for entering a recursive mutation.
As it turned out, it is rather easy to extend the existing listener
for structural changes to detect also value assignments. Actually
it seems we'd need both flavours, so be it.
Yeah, C++17, finally!
...not totally sure if we want to go that route.
However, the noise reduction in terms of code size at call site looks compelling
...while the first solution looked as a nice API, abstracting away
the actual collections (and in fact helped me to sport and fix a problem
with type substitution), in the end I prefer a simpler solution.
Since we're now passing in a lambda for transform anyway, it is
completely pointless to create an abstracted iterator type, just
for the sole purpose of dereferencing an unique_ptr.
As it stands now, this is all tightly interwoven implementation code,
and the DisplayFrame is no longer intended to become an important
interface on it's own (this role has been taken by the ViewHook /
ViewHooked types).
Note: as an asside, this solution also highlights, that our
TreeExplorer framework has gradually turned into a generic
pipeline building framework, rendering the "monadic use" just
one usage scenario amongst others. And since C++20 will bring
us a language based framework for building iteration pipelines,
very similar to what we have here, we can expect to retrofit
this framework eventually. For this reason, I now start using
the simple name `lib::explore(IT)` as a synonym.
the reason for the failure, as it turned out,
is that 'noexcept' is part of the function signature since C++17
And, since typically a STL container has const and non-const variants
of the begin() and end() function, the match to a member function pointer
became ambuguous, when probing with a signature without 'noexcept'
However, we deliberately want to support "any STL container like" types,
and this IMHO should include types with a possibly throwing iterator.
The rationale is, sometimes we want to expose some element *generator*
behind a container-like interface.
At this point I did an investigation if we can emulate something
in the way of a Concept -- i.e. rather than checking for the presence
of some functions on the interface, better try to cover the necessary
behaviour, like in a type class.
Unfortunately, while doable, this turns out to become quite technical;
and this highlights why the C++20 concepts are such an important addition
to the language.
So for the time being, we'll amend the existing solution
and look ahead to C++20
as it turns out, "almost" the whole codebase compiles in C++17 mode.
with the exception of two metaprogramming-related problems:
- our "duck detector" for STL containers does not trigger anymore
- the Metafunction to dissect Function sigantures (meta::_Fun) flounders
When drafting the time handling framework some years ago,
I foresaw the possible danger of mixing up numbers relating
to fractional seconds, with other plain numbers intended as
frame counts or as micro ticks. Thus I deliberately picked
an incompatible integer type for FSecs = boost::rational<long>
However, using long is problematic in itself, since its actual
bit length is not fixed, and especially on 32bit platforms long
is quite surprisingly defined to be the same as int.
However, meanwhile, using the new C++ features, I have blocked
pretty much any possible implicit conversion path, requiring
explicit conversions in the relevant ctor invocations. So,
after weighting in the alternatives, FSecs is now defined
as boost::rational<int64_t>.
GCC8 now spots and warns about such mismatches.
And we should take such warnings seriously;
code produced by the newer GCC versions tends to segfault,
especially under -O2 and above, when a return statement is
actually missing, even if the return value is actually not
used at call site.
Here, a functor to unlock the active "guard" is passed into
a macro construct, which basically allows to abstract the
various kinds of "guards", be it mutex, condition variable
or the like.
Seemingly, the intention was to deal with a failure when
unlocking -- however all the real implementations prefer
to kill the whole application without much ado.
Our diff language requires a diff to handle the complete contents of the target.
Through this clean-up hook this is now in fact enforced.
The actual reason for adding this however was that I need to ensure
listeners are triggered
As it turned out, the reason was a missing move-ctor.
The base of the whole DSL-Stack, TreeMutator, is defined MoveOnly,
and this is also the intended use (build an anonymous instance
through the DSL and move it into the work buffer prior to diff application)
However, C++ does *cease to define* a move ctor implicitly,
whenever /one of the "big five" is defined explicitly/.
So Detector4StructuralChanges was the culprit, it defined a dtor,
but failed to define the move ctor explicitly.
So.... well, this did cost me several hours to track down,
yet I still rather do not want to write all those ctors explicitly all the time,
and so I am still in favour of implicitly generated ctors, even if they hurt sometimes.
with the new decorator layer, we suddenly trigger a chain of template instantiation errors.
At first sight, they are almost undecipherable, yet after some experimentation, it becomes clear
that they relate down to the base class (TreeMutator), which is defined MoveOnly
This seems to indicate that, at some point in the call chain, we are
digressing from the move-construction scheme and switch over to copy construction,
which in the end failst (and shall fail).
Inconclusive, to be investigated further
basically the solution was a bit too naive and assumed everything is similar to a vector.
It is not, and this leads to some insidious problems with std::map, which hereby
are resolved by introducing ContainerTraits
All of the existing "simple" tests for the »Diff Framework« are way to much low-level;
they might indeed be elementary, but not introductory and simple to grasp.
We need a very simplistic example to show off the idea of mutation by diff,
and this simple example can then be used to build further usage test cases.
My actual goal for #1206 to have such a very basic usage demonstration and then
to attach a listener to this setup, and verify it is actually triggered.
PS: the name "GenNodeBasic_test" is somewhat pathetic, this test covers a lot
of ground and is anything but "basic". GenNode in fact became a widely used
fundamental data structure within Lumiera, and -- admittedly -- the existing
implementation might be somewhat simplistic, while the whole concept as such
is demanding, and we should accept that as the state of affairs
The population message is just made up, in order to create more interesting structures
in the UI and so to further the development of the timeline display.
For the actual structure I choose to mirror my example drawing in draw/UI-TimelineLayout-1.png
which is also used in the TiddlyWiki, on the #GuiTimelineView tiddler
https://lumiera.org/wiki/renderengine.html#GuiTimelineView
Mostly, std::regexp can be used as a drop-in replacement.
Note: unfortunately ECMA regexps do not support lookbehind assertions.
This lookbehind is necesary here because we want to allow parsing values
from strings with additional content, which means we need explicitly to
exclude mismatches due to invalid syntax.
We can work around that issue like "either line start, or *not* one of these characters.
Alternatively we could consider to make the match more rigid,
i.e we would require the string to conain *only* the timecode spec to be parsed.
The existing implementation created a Buffer-Type based on various traits,
including the constructor and destructor functions for the buffer content.
However, this necessitates calculating the hash_value of a std::function,
which (see #294) is generally not possible to implement.
So with this changeset we now store an additional identity hash value
right into the TypeHandler, based on the target type placed into the buffer
<rant>
the "improved" boost::rational can no longer compute 1/x
quite brilliant
</rant>
well... the reason is again signed vs unsigned int.
FrameRate is based on unsigned int (since a negative frame rate makes no sense).
seemingly, the newer boost libraries added an internal type rational<I>::bool_type
together with an overload for the equality comparison operator.
Unfortunately this now renders a comparison ambiguous with the constant zero (i.e. int{0})
because in our use case we employ rational<uint>.
Workaround is to compare explicitly to a zero of the underlying integer type.
the template lib::PolymorphicValue seemingly picked the wrong
implementation strategy for "virtual copy support": In fact it is possible
to use the optimal strategy here, since our interface inherits from CloneSupport,
yet the metaprogramming logic picked the mix-in-adapter (which requires one additional "slot"
of storage plus a dynamic_cast at runtime).
The reason for this malfunction was the fact that we used META_DETECT_FUNCTION
to detect the presence of a clone-support-function. This is not correct, since
it can only detect a function in the *same* class, not an inherited function.
Thus, switching to META_DETECT_FUNCTION_NAME solves this problem
Well, this solution has some downsides, but since I intend to rewrite the
whole virtual copy support (#1197) anyway, I'll deem this acceptable for now
TODO / WIP: still some diagnostics code to clean up, plus a better solution for the EmptyBase
...which, in the end, can even be considered the more logical design choice,
since the "verb visitor" is a more elaborated and sophisiticated Verb-Token,
adding the special twist of embedded storage for variable function arguments
...but bad news on the main issue:
the workaround consumes the tuple and thus is not tenable!
And what is even worse: the textbook implementation of std::apply is
equivalent to our workaround and also consumes the argument tuple
A simple yet weird workaround (and basically equivalent to our helper function)
is to wrap the argument tuple itself into std::forward<Args> -- which has the
effect of exposing RValue references to the forwarding function, thus silencing
the compiler.
I am not happy with this result, since it contradicts the notion of perfect forwarding.
As an asside, the ressearch has sorted out some secondary suspicions..
- it is *not* the Varargs argument pack as such
- it is *not* the VerbToken type as such
The problem clearly is related to exposing tuple elements to a forwarding function.
this is a generalisation of what we use in the diff framework;
typically you'd package the VerbToken into some kind of container,
together with the concrete invocation argument.
However, the specific twist here is that we want *variable arguments*,
depending on the actual operation called on the interpreter interface.
Up to now, PolymorphicValue was always used as-is, packaged into a typedef.
Now we consider using it as building block within an adapter for visitor-like tokens.
Which requires to pass-down the ctor call directly from the subclass, at least if we
want to emplace the resulting entity directly into a stdlib container.
As an asside, PolymorphicValue also used explicit specialisations for N-arguments,
which meanwhile can be replaced by variadic templates
...which leads to a specific twist here; while in the simple version
we still could hope to get away with a simple uniform uint argument,
the situation has changed altogether now. The canvas has turned into
some generic component, since it is instantiated two times, onece for
the time ruler and once for the actual body content. Thus all of the
specifics of the drawing code need to be pushed into a new, dedicated
renderer component. And this more or less forces us to pass all the
actual presentation variations through the invocation arguments of
the visitor.
So we're now off again for a digression, we need a more generalised visitor
effectively we rely in the micro tick timescale promoted by libGAVL,
but it seems indicated to introduce our own constant definition.
And also clarify some comments and tests.
(this changeset does not change any values or functionality)
basically we can pick just any convention here, and so we should pick the convention in a way
that makes most sense informally, for a *human reader*. But what we previously did, was to pick
the condition such as to make it simple in some situations for the programmer....
With the predictable result: even with the disappointingly small number of usages we have up to now,
we got that condition backwards several times.
OK, so from now on!!!
Time::NEVER == Time::MAX, because "never" is as far as possible into the future
A classical carry-over of dirty values...
Problem arises, when starting an unconditional wait on the same object monitor,
which previously conducted a timed wait. Then the obsolete timeout from the previous
wait remained in place, causing our Sync-Wrapper (erroneously) to assume a timed wait
and then pthread to return immediately from this timed wait.
The result was permanent idle looping in the ProcDispatcher, after the first command was processed
When invoking the util::toString conversion, we indeed to want any conversion,
including explicit conversion operators. However, probing the possibility to build a string
can be dangerous, since there is a string constructor from characters, and
integral types can be converted to characters.
OTOH, leaving out explicit conversions is likewise not desirable, since there are
class types, which deliberately do not offer an implicit conversion, but allow
explicit conversion for dump and diagnostic output. The notorious example for
such a situation is the lib::idi::EntryID<TY>. We certainly do not want an
EntryID to be converted into a string without further notice, but we do want
an EntryID to be automatically rendered to string in diagnostic output, since
this will include the human readable ID part.
See especially: 8432420726
Now we'll attempt to get out of this dilemma by probing explicitly for the presence
of a string conversion operator, which will fail for any non-class types, thereby
ruling out all those nasty indirect type -> character -> string conversion paths.
The rationale is: if someone queries the predicate can_convertToString, the intention
is really to get an string rendering, and not just to invoke some random function
with an string argument.
- most notably the NOBUG logging flags have been renamed now
- but for the configuration, I'll stick to "GUI" for now,
since "Stage" would be bewildering for an occasional user
- in a similar vein, most documentation continues to refer to the GUI
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
same pattern as the existing EntryID, i.e. a human readable symbol plus a hash
but the hash is just random, instead of deriving it from the symbol text.
Use case is when we explicitly need a distinct identity, even when the
human readable symbolic name is the same. Actual example: the fork root in the timeline
...it should have been explicit from start, since there is no point
in converting an EntryID into a plain flat string without further notice
this became evident, when the compiler picked the string overload on
MakeRec().genNode(specialID)
...which is in compliance to the rules, since string is a direct match,
while BareEntryID would be an (slicing) upcast. However, obviously we
want the BareEntryID here, and not an implicit string conversion,
thereby discarding the special hash value hidden within the ID
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
Problem is, the InteractionDirector, being the representation of the model root,
needs to manage and maintain the collection of "timelines". However, these
can not be widgets, rather, they need to attach to widgets living within
the GUI widget structure proper, i.e. within the TimelinePanel
proposed solution is to build a smart handle based on WLink,
but also delegating the DiffMutable interface
...these magical strings are already spreading dangerously throughout the code base
PS: also fixup for c6b8811af0 (broken whitespace in test definition)
As it turns out, several problems reinforce each other
- lumiera error does not properly propagate the cause message
- our test/dummy code does not check the ExecResult
- thus the exception is detected rather accidentally, when entring the next sync/wait state
- emergency shutdown is chaotic in its very nature (this is well known...)
- but especially triggerShutdown is not airtight and might die...
- causing the shutdown to hang....
And last but not least, a ZombieCheck tripwire got triggered,
but unfortunately I was unable to get hold of the zombie iteself
test_meta_markAction always produces a state mark with payload type string.
However, the model::Tangible expects a bool payload when handling the "expand" mark.
- add diagnostics to lib::variant to indicate expected and actual payload type
- attempt to fix with boost::lexical_cast; this is insufficient, since
you'd expect such a function to understand "true" and "false" etc.
Moreover, raising this exception causes emergency shutdown, which
flounders due to triggering a ZombieCheck. Interesting.
well... reduction in size of the debug build objects
turns out not to be so large as I hoped. But it is significant anyway,
about 3-4MB on the most affected test classes. Plus from now on we
do not repeat that code on other tests using the same features.
up to now, EventLog was header only, which seems to cause
a significant bloat in terms of generated code size, especially
in debug builds. One major source for this kind of "template bloat"
is the IterChainSearch, rsp. the filter and transformer iterators.
And since EventLog is not meant for performance critical application code,
but rather serves as helper for writing unit tests, an obvious remedy is
to move that problematic part of the code down into a dedicate translation
unit, instead of using inline functions. To prepare this refactoring,
some var arg (templated) API funcitons need to be segregated.
For the before / after chaining search functions,
we now do one single step in the respective direction before evaluating
the new (next) filter condition. However, we also need to *deactivate* the
previous condition, otherwise that single "step" might cause us to jump
or even exhaust the underlying filter, due to the old filter condition
still being applied.
due to the lack of real backtracking, the existing solution
relied on a quirk, and started the before / after chained search
conditions /at/ the current element, not after / before it.
Now we're able to remove this somewhat surprising behaviour, yet to do so
we also need to introduce basic "just search" variations of all search
operations, in order to define the initial condition for a chained search.
Without that, the first condition in a chain would never be able to
match on the header entry of the log
- need to use dedicated steps in the chain for every added condition now
- seems to break the logic on tests on non-match.
This doesn't come as a surprise, since backtracking can be expected
to reveal additional solutions.
NOTE: some tests broken, to be investigated
est-event-log-test.cpp:228: thread_1: verify_callLogging: (log.ensureNot("fun").after("fun").after("fun2"))
...which can be achieved by checking the backtracking loop
always right after the non-backtracking iteration, exploiting
the fact that the guard conditions of both are complimentary.
So the only case when we'd actually enter the backtracking
loop after regular iteration would precisely be when
we drop down due to exahausting an upper layer.
The result now reads
"sausage-bacon-tomato-and-spam-spam-bacon-spam-tomato-and-spam-bacon-tomato-and-bacon-tomato-and-tomato-and"
...which sounds correct, yay!
...since usually such evaluation layers are finally wrapped into
an IterableDecorator and then presented as TreeEplorer -- an exercise
we do not want to perform here, since it is pointless in the typicall
use case. The IterChainSearch is already meant to be ready-for-use.
Thus, instead of wrapping again, the pragmatic solution is simply
to overload the missing operator++ and make it call the augmented
iterNext() function. Related to this, we also need to ensure
proper operation in case no further expansion is mandated
...seems basically sane now.
Just we still need to wrap it one more time into IterableDecorator;
which means the overall scheme how to build and package the whole pipeline
is not correct yet.
Maybe it is not possible to get it packaged all into one single class?
