Re: Issue 5656: Warn about accessing the Global context explicitly (issue 567050043 by address@hidden)

[nine . fierce . ballads]

Reviewers: thomasmorley651,

Message:
On 2020/01/09 00:33:35, thomasmorley651 wrote:
> I know several custom codings (defining a new grob, thus the need to
reintroduce
> all-grob-descriptions at Global level) using

> \layout {
>    \context {
>      \Global
>      \grobdescriptions #all-grob-descriptions
>    }
> }

> will this be disallowed with this patch?
> Then I'd object.

This patch doesn't touch context definitions, so I don't expect anything
about them to change.

> If it stays allowed, why disallow \context Global etc then?

*Shrug*  I'm trying to understand what was intended, cover it in tests,
and fix it if necessary.  In the future, I'd like to improve the
maintainability of the code.  If I've misinterpreted how this should
work, I'm open to correction.

This is the description of the Global context:
    \description "Hard coded entry point for LilyPond.  Cannot be
tuned."
Does that mean that \set Global.whatever = ... should be disallowed?  It
sounds like it, and it seems not to work in master, and the warning
behavior is inconsistent.  Try this example, if you wish.

\version "2.21.0"

\layout {
  \context {
    \Global
    \unset instrumentName
  }
  \context {
    \Score
    \unset instrumentName
  }
  \context {
    \Staff
    \unset instrumentName
  }
}

\context Global {
  %% This has no apparent effect and there is no warning.
  \set Global.instrumentName = "Global 1"
  a'1
}

\context Score {
  b'1
}

\context Score {
  %% This issues a warning but engraves the name anyway.  I think it
  %% falls back to setting the property in the current context
  %% (Score), but I haven't verified that.
  \set Global.instrumentName = "Global 2"
  c''1
}

\context Score {
  d''1
}

\context Score {
  \set Score.instrumentName = "Score"
  e''1
}

Regards,
--
Dan

Description:
https://sourceforge.net/p/testlilyissues/issues/5656/

This is another nudge in the direction of consistency in handling
contexts.

1: Add regression test for \new Global
This passes without changes: LilyPond issues a warning about \new
Global.

2: Warn about trying to access the Global context explicitly
Warn for \context Global and \set Global.property as for \new Global.

Please review this at https://codereview.appspot.com/567050043/

Affected files (+38, -7 lines):
  A input/regression/context-global-find.ly
  A input/regression/context-global-new.ly
  M lily/context.cc
  M lily/include/context.hh
  M lily/include/global-context.hh


Index: input/regression/context-global-find.ly
diff --git a/input/regression/context-global-find.ly  
b/input/regression/context-global-find.ly
new file mode 100644
index  
0000000000000000000000000000000000000000..fd50457cb0e814bb2e3e1bbcfaad309b957a4f96
--- /dev/null
+++ b/input/regression/context-global-find.ly
@@ -0,0 +1,16 @@
+\version "2.21.0"
+
+\header {
+  texidoc = "User code is not allowed to access the Global context.
+  The visual output of this test is not important."
+}
+
+#(ly:set-option 'warning-as-error #t)
+%% once for \context and once for \set
+#(ly:expect-warning (_ "cannot find or create context: ~a") 'Global)
+#(ly:expect-warning (_ "cannot find or create context: ~a") 'Global)
+
+\context Global {
+  \set Global.instrumentName = "Global"
+  s1
+}
Index: input/regression/context-global-new.ly
diff --git a/input/regression/context-global-new.ly  
b/input/regression/context-global-new.ly
new file mode 100644
index  
0000000000000000000000000000000000000000..fceb6fe4fc5d9624b9b2724c18e2aae3643101bf
--- /dev/null
+++ b/input/regression/context-global-new.ly
@@ -0,0 +1,11 @@
+\version "2.21.0"
+
+\header {
+  texidoc = "User code is not allowed to create a Global context.
+  The visual output of this test is not important."
+}
+
+#(ly:set-option 'warning-as-error #t)
+#(ly:expect-warning (_ "cannot create context: ~a") 'Global)
+
+\new Global s1
Index: lily/context.cc
diff --git a/lily/context.cc b/lily/context.cc
index  
9f6ec8ebdb8184b78a859916e3d46b5ad747dc7d..b739a020de8ae62adb5c2c40a0c34d15c6f588d3  
100644
--- a/lily/context.cc
+++ b/lily/context.cc
@@ -132,6 +132,13 @@ Context *
 Context::find_create_context (SCM n, const string &id,
                               SCM operations)
 {
+  // Searching below in recursive calls can find contexts beyond those  
that are
+  // visible when looking just down and up from the starting point.  The
+  // regression test lyric-combine-polyphonic.ly has a reasonably simple
+  // example of how this can be useful.
+  if (Context *existing = find_context_below (this, n, id))
+    return existing->is_accessible_to_user () ? existing : nullptr;
+
   /*
     Don't create multiple score contexts.
   */
@@ -145,13 +152,6 @@ Context::find_create_context (SCM n, const string &id,
         }
     }

-  // Searching below in recursive calls can find contexts beyond those  
that are
-  // visible when looking just down and up from the starting point.  The
-  // regression test lyric-combine-polyphonic.ly has a reasonably simple
-  // example of how this can be useful.
-  if (Context *existing = find_context_below (this, n, id))
-    return existing;
-
   vector<Context_def *> path = path_to_acceptable_context (n);
   if (!path.empty ())
     return create_hierarchy (path, "", id, operations);
Index: lily/include/context.hh
diff --git a/lily/include/context.hh b/lily/include/context.hh
index  
d09c6956a474b794a93a70e632069c3dc68f566a..cd0d882172fb57278c433a9ea2ca7a9e7c38ceb1  
100644
--- a/lily/include/context.hh
+++ b/lily/include/context.hh
@@ -135,6 +135,8 @@ public:
   void internal_set_property (SCM var_sym, SCM value);

   Context *create_context (Context_def *, const string&, SCM);
+  virtual bool is_accessible_to_user () const { return true; }
+
   void create_context_from_event (SCM);
   void acknowledge_infant (SCM);
   void remove_context (SCM);
Index: lily/include/global-context.hh
diff --git a/lily/include/global-context.hh b/lily/include/global-context.hh
index  
8fc83daca796294cfb2a1e777ee557d53efb18ca..7dc36ce4afc659d6ca55c059167f3e296a815478  
100644
--- a/lily/include/global-context.hh
+++ b/lily/include/global-context.hh
@@ -43,6 +43,8 @@ public:
   Moment sneaky_insert_extra_moment (Moment);
   void add_moment_to_process (Moment);
   void run_iterator_on_me (Music_iterator *);
+
+  bool is_accessible_to_user () const override { return false; }
   Context *get_score_context () const override;

   void apply_finalizations ();