Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

To attribute #2465

Merged
merged 7 commits into from
Jan 8, 2025
Merged

To attribute #2465

merged 7 commits into from
Jan 8, 2025

Conversation

brucemiller
Copy link
Owner

@brucemiller brucemiller commented Dec 31, 2024

This PR provides an option toAttribute to DefConstructor,DefEnvironment (and Whatsits generally) to customize the conversion of a whatsit to an attribute value; its sytax is analogous to that of the reversion option, being a sub or string, possibly containing #1...
This is useful for objects which need the flexibility of a Whatsit (as opposed to simple Box) but which are sometimes placed in an attribute value that ends up in the text (or html) of the conversion; In those cases, the value should contain neither XML, nor the TeX reversion (as toString typically does). The motivating example is \hskip.
The setAttribute method of Document, when given an object reference as value, will use toAttribute to turn it into a string, but we need to be careful not to use ToString on such values before they get there.

@brucemiller brucemiller requested a review from dginev December 31, 2024 16:00
Copy link
Collaborator

@dginev dginev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting upgrade, looks healthy.

I have a naming comment (no surprise there). The toNoun names are typically used for methods in latexml. The alternative form specifiers tend to have some noun name as with reversion, alias, sizer...

Indeed, toAttribute names methods implemented by our Box-like objects. I think it would be helpful to name the config parameter in the Definition objects slightly differently.

(Aside: one of my related questions was wheter we want a generic ->toAttribute method in Definition.pm, which defaults to ->toString)

A simple distinction would be to swap to for as, namely asAttribute. Another is intoAttribute (Rust has standardized .into() as one reason I thought of that).

The main point of my comment is that the distinction between reversion and revert() is a nice model to follow for ? and toAttribute(). Maybe attributeSpec or attributeForm gets even closer to that...

@@ -1376,8 +1376,14 @@ sub makeError {
# [xml:id and namespaced attributes are always allowed]
sub setAttribute {
my ($self, $node, $key, $value) = @_;
return if (ref $value) && ((!blessed($value)) || !$value->can('toAttribute'));
$value = $value->toAttribute if ref $value;
return if (ref $value) && ((!blessed($value)) || !$value->can('toAttribute'));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be useful to emit a Warning if this unusual case is encountered, where $value was e.g. a perl array ref or hash ref? Could be some rare mistake that gets caught quickly with a warning message.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea; done!

elsif (ref $spec eq 'CODE') { # If handled by CODE, call it
$spec = &$spec($self, $self->getArgs); }
# Now, similar to substituteParameters, but creating a string.
$spec =~ s/#(#|[1-9]|\w+)/ toAttribute_aux($self,$1)/eg;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I think I understand the code here, but I am wondering if we should have a an example DefConstructor that uses this argument-based toAttribute string form, and a test that exercises it.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The conceit is that it should work just like reversion.

$value = $self->getArg(ord($code) - ord('0')); }
else {
$value = $self->getProperty($code); }
$value = $value->toAttribute if (defined $value) && (ref $value);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor, but ref undef works fine, so defined $value is redundnat.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed; fixed.

(the default is C<toString>).
The I<textstring> string can include C<#1>, C<#2>...
The I<code> is called with the C<$whatsit> and digested arguments
and must return a string.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly a doc string like this is also needed for the DefEnvironment addition.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, done.

@brucemiller
Copy link
Owner Author

Yeah, the naming is generally off for all these toNoun things, but I like your attributeForm suggestion.

@brucemiller brucemiller merged commit fa8191e into master Jan 8, 2025
26 checks passed
@brucemiller brucemiller deleted the toAttribute branch January 8, 2025 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants