Re: Remove ly:grob-properties (issue 549840044 by hanw...@gmail.com)
commit 7688936a4e598bc9919e8a9adceb3ba1c9b2c23c Author: Han-Wen Nienhuys Date: Sat Apr 11 17:49:38 2020 +0200 Remove ly:grob-properties https://codereview.appspot.com/549840044/
Re: Remove ly:grob-properties (issue 549840044 by hanw...@gmail.com)
On 2020/04/11 17:44:11, hanwenn wrote: > On 2020/04/11 17:19:19, dak wrote: > > > I thought of removing the other function too, and I agree in principle, but > it > > > can only break more user files. As long as we're not considering > reorganizing > > > the immutable lists, I think it can stay. > > > > Note that there is no necessity of returning a bonafide part of the grob > > structure: one can construct a list of properties on the fly. Particularly > > because this is used read-only and only occasionally. > > I know. But if we make a copy, that will be expensive too. It's either imprudent user code or code that actually potentially uses all the properties. Either way I'd not be worried about the cost. That we don't want to use it in the internals for regular use then is obvious. https://codereview.appspot.com/549840044/
Re: Remove ly:grob-properties (issue 549840044 by hanw...@gmail.com)
On 2020/04/11 17:19:19, dak wrote: > > I thought of removing the other function too, and I agree in principle, but it > > can only break more user files. As long as we're not considering reorganizing > > the immutable lists, I think it can stay. > > Note that there is no necessity of returning a bonafide part of the grob > structure: one can construct a list of properties on the fly. Particularly > because this is used read-only and only occasionally. I know. But if we make a copy, that will be expensive too. Any comment on NOSUBMIT: this is an incompatible change; where are the release notes? https://codereview.appspot.com/549840044/
Re: Remove ly:grob-properties (issue 549840044 by hanw...@gmail.com)
On 2020/04/11 16:38:28, hanwenn wrote: > On 2020/04/11 16:02:31, dak wrote: > > The description says "[ly:grob-properties] will not return the properties that > > were \overridden." > > > > Aren't you confusing this with ly:grob-basic-properties ? I think > > ly:grob-properties will actually _only_ return the properties that were > > overriden. > > \override pushes data onto the immutable list (or at least, it used to be like > that). In effect you are right (and I misremembered), but override pushes onto context-maintained lists which are ultimately used as template when make_grob gets called and _then_ the overriden properties end up on the immutable list. > I thought of removing the other function too, and I agree in principle, but it > can only break more user files. As long as we're not considering reorganizing > the immutable lists, I think it can stay. Note that there is no necessity of returning a bonafide part of the grob structure: one can construct a list of properties on the fly. Particularly because this is used read-only and only occasionally. https://codereview.appspot.com/549840044/
Re: Remove ly:grob-properties (issue 549840044 by hanw...@gmail.com)
> Fixes > Documentation/snippets/overriding-articulations-of-destinct-type, > which should be mirrored back into LSR. Fixed in LSR https://codereview.appspot.com/549840044/
Re: Remove ly:grob-properties (issue 549840044 by hanw...@gmail.com)
Reviewers: dak, thomasmorley651,
Message:
On 2020/04/11 16:02:31, dak wrote:
> The description says "[ly:grob-properties] will not return the
properties that
> were \overridden."
>
> Aren't you confusing this with ly:grob-basic-properties ? I think
> ly:grob-properties will actually _only_ return the properties that
were
> overriden.
\override pushes data onto the immutable list (or at least, it used to
be like that). So if you do \override Stem.length = #5 , you won't see
'length in the return value of ly:grob-properties
I thought of removing the other function too, and I agree in principle,
but it can only break more user files. As long as we're not considering
reorganizing the immutable lists, I think it can stay.
Description:
Remove ly:grob-properties
This function is hard to use correctly:
* one cannot set the head of the alist, so it cannot be used for
modifications.
* it will not return the properties that were \overridden
Instead, ly:grob-property should be used.
This paves the way to further work on (grob) property storage
mechanisms.
Fixes
Documentation/snippets/overriding-articulations-of-destinct-type,
which should be mirrored back into LSR.
NOSUBMIT: this is an incompatible change; where are the release notes?
Please review this at https://codereview.appspot.com/549840044/
Affected files (+1, -13 lines):
M Documentation/snippets/overriding-articulations-of-destinct-type.ly
M lily/grob-scheme.cc
Index: Documentation/snippets/overriding-articulations-of-destinct-type.ly
diff --git
a/Documentation/snippets/overriding-articulations-of-destinct-type.ly
b/Documentation/snippets/overriding-articulations-of-destinct-type.ly
index
5053be73b1161970f5d7cc47baf729fa92c096bb..525840d718a5f32f0ba4b15b66dc72b550305fbf
100644
--- a/Documentation/snippets/overriding-articulations-of-destinct-type.ly
+++ b/Documentation/snippets/overriding-articulations-of-destinct-type.ly
@@ -28,7 +28,7 @@ With 2.16.2 it is possible to put the proposed function,
#(define (custom-script-tweaks ls)
(lambda (grob)
(let* ((type (ly:prob-property
-(assoc-ref (ly:grob-properties grob) 'cause)
+(ly:grob-property grob 'cause)
'articulation-type))
(tweaks (assoc-ref ls type)))
(if tweaks
Index: lily/grob-scheme.cc
diff --git a/lily/grob-scheme.cc b/lily/grob-scheme.cc
index
a078e7ac083d237169b39c15f8b39dc3293ee44b..22f89c87e8d48eb6e8db231a3066b2fcfeaddeca
100644
--- a/lily/grob-scheme.cc
+++ b/lily/grob-scheme.cc
@@ -312,18 +312,6 @@ LY_DEFINE (ly_grob_set_parent_x, "ly:grob-set-parent!",
return SCM_UNSPECIFIED;
}
-LY_DEFINE (ly_grob_properties, "ly:grob-properties",
- 1, 0, 0, (SCM grob),
- "Get the mutable properties of @var{grob}.")
-{
- Grob *g = unsmob (grob);
-
- LY_ASSERT_SMOB (Grob, grob, 1);
-
- /* FIXME: uhg? copy/read only? */
- return g->mutable_property_alist_;
-}
-
LY_DEFINE (ly_grob_basic_properties, "ly:grob-basic-properties",
1, 0, 0, (SCM grob),
"Get the immutable properties of @var{grob}.")
Re: Remove ly:grob-properties (issue 549840044 by hanw...@gmail.com)
On 2020/04/11 16:32:58, thomasmorley651 wrote: > On 2020/04/11 16:31:48, thomasmorley651 wrote: > > > Btw, it's used in > > input/regression/multi-measure-rest-reminder.ly > > input/regression/scheme-text-spanner.ly > > as well. > > No, that's ly:grob-properties? > Bad grepping ... Also icky name overlaps. ly:grob-properties? was last. Maybe it should be ly:context-grob-properties? instead? It's the thing contexts use for tracking grob properties. https://codereview.appspot.com/549840044/
Re: Remove ly:grob-properties (issue 549840044 by hanw...@gmail.com)
On 2020/04/11 16:31:48, thomasmorley651 wrote: > Btw, it's used in > input/regression/multi-measure-rest-reminder.ly > input/regression/scheme-text-spanner.ly > as well. No, that's ly:grob-properties? Bad grepping ... https://codereview.appspot.com/549840044/
Re: Remove ly:grob-properties (issue 549840044 by hanw...@gmail.com)
The change in the snippet is ofcourse nice. Though, please don't delete ly:grob-properties or ly:grob-basic-properties. I use them every day. They are tools to get insights at the grob at hand. Don't judge from our code base and the few occurencies there. Btw, it's used in input/regression/multi-measure-rest-reminder.ly input/regression/scheme-text-spanner.ly as well. Again, it's in every days use!!! https://codereview.appspot.com/549840044/diff/567430043/Documentation/snippets/overriding-articulations-of-destinct-type.ly File Documentation/snippets/overriding-articulations-of-destinct-type.ly (right): https://codereview.appspot.com/549840044/diff/567430043/Documentation/snippets/overriding-articulations-of-destinct-type.ly#newcode31 Documentation/snippets/overriding-articulations-of-destinct-type.ly:31: (ly:grob-property grob 'cause) Yep, that's better here. https://codereview.appspot.com/549840044/
Re: Remove ly:grob-properties (issue 549840044 by hanw...@gmail.com)
The description says "[ly:grob-properties] will not return the properties that were \overridden." Aren't you confusing this with ly:grob-basic-properties ? I think ly:grob-properties will actually _only_ return the properties that were overriden. https://codereview.appspot.com/549840044/
Remove ly:grob-properties (issue 549840044 by hanw...@gmail.com)
https://codereview.appspot.com/549840044/diff/567430043/lily/grob-scheme.cc File lily/grob-scheme.cc (left): https://codereview.appspot.com/549840044/diff/567430043/lily/grob-scheme.cc#oldcode327 lily/grob-scheme.cc:327: LY_DEFINE (ly_grob_basic_properties, "ly:grob-basic-properties", There is really no point in removing ly:grob-properties while retaining ly:grob-basic-properties as they are a pair of accessors. https://codereview.appspot.com/549840044/
