Helper api

This pull request is here to serve as a place to discuss and review proposed API enhancements for higher level APIs as discussed in #178. This branch will be rebased and reworked multiple times. I'll push to it periodically when I have something in a reviewable state.

@sahilarora535 @smilingthax Here is the pull request I mentioned in #178. At the time of this writing, I have pushed up changes to support a page-level API using the helper approach. Questions and comments welcome.

Hmm, QPDFPageDocumentHandler::getAllPages() (still) returns a vector<QPDFObjectHandle>, whereas I'd most certainly would want / "need" a vector<QPDFPageObjectHelper>...(?)
One of the strengths of the new paradigm, as I currently understand it, would be that e.g. the page cache would finally become part of the QPDFPageDocumentHandler -- i.e. the Handler becomes the "container" for all the internal bookkeeping used to implement certain operations ... (?)
Other than that the concept looks good, and I'm looking forward to seeing how well this works for the form stuff.
(minor sidenote -- probably not solvable without major changes to libqpdf: You might have seen the node-libqpdf binding (on my github) I experimented with a few months ago... I found that it is nearly impossible to create a resource-managing wrapper ("smart pointer") around a QPDF object ... the new classes add just more of those internal references to QPDFs that are hard to track...)

Regarding your comment about getAllPages returning a vector of QPDFPageObjectHelper, I definitely agree that that would be a more natural interface. I may put this in a comment, or I may provide an alternative interface. The reason I ended up not doing it stems from a design decision we made a long time ago: getAllPages actually returns a reference to an internal structure that is updated dynamically by things like addPage. In order to make all those semantics continue to work, we would have to double work. There are comments in the code that talk about this, and there are a number of tests the explicitly exercise the mutability of the vector returned.
Personally I think this is a bad pattern, and I'm not sure why we decided to do it this way. I would be surprised if many people are relying on this behavior or would consider it the right behavior.
Maybe I should change the method in QPDFPageDocumentHelper to return vector<QPDFPageObjectHelper and change the one that calls the other method to something like getMutablePageObjectVector for emphasis. It would be an easy change.

Actually, I am not going to expose the old getAllPages or the update cache method in QPDFPageDocumentHelper. I am going to push up a new version in a few minutes for your comments. Your input is very valuable. I thought of this while I was doing, but your comment is compelling me to fix it. The result is going to be better because of your review and comment.

Well, I deliberately ignored any legacy constraints that could also play a role in my comment.
One way (you seem to be heading in another direction) would be to put all mutators that also affect the cache (like addPage) (only) on the QPDFPageObjectHelper. Basically the legacy API on QPDF would have to internally use a QPDFPageObjectHelper as well...
But cache invalidate is inherently a hard problem. At some point one has to rely on the programmer to (e.g.) not  use a QPDFPageObjectHelper after he loaded an new pdf file into the parent QPDF (or at least to call the page cache invalidation method).

I don't want to get distracted by reworking the page API. I would surely create extra work in maintaining compatibility. Unfortunately, certain API and design decisions stick around, and we get stuck with them. I will provide a newer, preferred interface for future use, and use this to validate the helper pattern. When we work out the pattern for forms, we will not repeat those mistakes.
But you definitely looking at this the right way, and your comments are validating my approach. It is in fact the case that state handling and so forth can be confined to the helper classes. It was a mistake to ever expose an internal cache. Had the cache been kept private, and had getAllPages returned a vector instead of a vector const&, we would have been in better shape.

From a programmer perspective, it'd be worse, when there is an internal cache, but I can't invalidate it, although it has become stale. In the new approach it would make sense that I have to create a new QPDFPageDocumentHelper after manually tinkering with some of the underlying pdf objects.
The unexpected thing with it currently being a const & is that it could change under my feet.
OTOH, other programming languages tend towards an iterator-based interface for thinks like getAllPages - supported by language features like generators / coroutines and spreads / expansion...

I'm almost done with the changes. The new interface won't expose a cache. I think it will be better.

The cache is still exposed by the older methods. I can't do anything about that.

Changes pushed. Care to take a fresh look?

We can always come back to the page stuff, as we gain new insights from the form stuff. I have no further comments for now.

Thanks. As soon as make check finishes, I'll push up some fixups for the last around of commits that get rid of extraneous copies and conversions between object handle and object helper. Thanks for your comments. I'm going to take a first crack at defining some method signatures for forms tonight before I stop so I can hit the ground running on those tomorrow.

Assuming everything goes well, my next push will squash some of these commits.

@smilingthax I have pushed a series of commits with the first iteration of a proposed API for form helpers. There are no setters yet, just methods to query stuff. It should not take me long at all to implement what I have so far. Then I will see about adding a few methods to set some things such as field values or appearance streams.

Re: "TBD: where do methods to alter field and annotation values go"
This is where it gets tricky...

It might make sense to put those methods that can be executed locally onto the QPDFFormFieldObjectHelper, and only the other ones on QPDFAcroFormDocumentHelper... -- esp. things like setValue(): As a programmer it feels a bit unnatural to require two different objects for get vs. set... one might inquire: "Why aren't they just all on QPDFAcroFormDocumentHelper, when that's where the setter has to be?" ... I do understand your desire to not reference the DocumentHelper from ObjectHelper; OTOH, a QPDFObjectHandle does know the QPDF it came from (e.g. for stream content, I think) after all...
Now, in most cases /NeedAppearances has to become true, thus there will be no setters left on the ObjectHandle ... unless we change the design and do reference the DocumentHelper (or at least "certain important QPDFObjectHandles" ...) ... hmm... I'm quite undecided...

A few use cases to think through ... let's fast forward to the point where qpdf would support creating new forms from scratch:
1a. user first creates a new form field, possibly not at the root, but below some QPDFObjectHandle / QPDFFormFieldObjectHelper
-> (e.g.) QPDFFormFieldObjectHelper QPDFAcroFormDocumentHelper::createField(const string& name, ..., QPDFFormFieldObjectHelper *parent = nullptr)
1b. alternatively, user reuses an existing form field
2. user creates a new form(widget) annotation - on a certain page (or: must be added later to exactly one page ?)
-> probably QPDFAnnotationObjectHelper QPDFAcroFormDocumentHelper::createAnnotation(const QPDFFormFieldObjectHelper &field)  [NOTE: const & or by value ?] and
QPDFAcroFormDocumentHelper::addAnnotationToPage(QPDFFormFieldObjectHelper &field, const QPDFPageObjectHelper &page)
(or a method that does both at once).
To remove an annotation or field, things would go the other way round.

What happens when the user tries to remove a form field that is still associated with an annotation?
A merged field+annotation dictionary means that a selective removal of only the annotation means removal of a certain set of keys from the dictionary.
Adding a second annotation to an existing field also requires extracting those keys into a separate dictionary.
Maybe: additional method that "removes field and all associated annotations".

Re: "QPDFFormFieldObjectHelper::getQuadding" ...
-> Seems to make sense: When implementing the appearance stream (re-)generator, it either has to look up each key it needs via (e.g.) ...ObjectHelper::getWithInheritance(key,default=QPDF_Null), or we add a specific method for each key we're interested in (and that's specifically marked as inheritable in the pdf spec!)... at least, we can transparently put caching logic (analogous to the "push inherited fields to pages" implementation) into QPDFAcroFromDocumentHelper, if we want to.

Re: "QPDFObjectHandle::Rectangle"
This rings a bell...
In pdftopdf, I basically use Rectangle QPDFPageObjectHelper::getMediaBox(), getCropBox()  (i.e. if (hasKey(/CropBox)) return /CropBox; else return /MediaBox;), and similar for getTrimBox/getArtBox ...
I also have makeBox(double x0,y0,x1,y1) ...

Re: "TBD: where do methods to alter field and annotation values go"

You're thinking in the right way. I'm pretty sure they will go in the doc helper because most of them will need to go there and, as you say, it's not good for the programmer to have to go to two places. Regarding the object handle knowing the QPDF it comes from, this is not true of direct objects that are created by the programmer until they are actually hooked in, so I don't want to ever rely on this.

When implementing the appearance stream (re-)generator, it either has to look up each key it needs via (e.g.) ...ObjectHelper::getWithInheritance(key,default=QPDF_Null), or we add a specific method for each key we're interested in (and that's specifically marked as inheritable in the pdf spec!)

I fully expect to provide a method to look things up with inheritance and also to provide a few helper methods for specific, common cases. A method to push inheritable things down may be useful at some point, but as you point out, we can't flatten the structure without potentially breaking fully qualified names.
Right now, I am writing the code to assume that an annotation can be referenced by exactly one field, which is what the spec says. If an annotation is referenced by more than one field, I will warn and just keep one of them arbitrarily. As I traverse the fields tree, I am going to sanity check /Parent. If /Parent is absent and shouldn't be absent, I'm going to set it. If it is present and set to the wrong value, I'm going to modify it and give a warning. I am going to take care not to add /Parent keys to annotation dictionaries that are not merged with field dictionaries. The main purpose of all of this is that it makes it safe to walk up the parent structure for inheritable resources. While everything that can go wrong does go wrong in the wild somewhere, I suspect inconsistencies in the tree structure will be very rare in the wild since mostly things won't work if you do that.

I also have makeBox(double x0,y0,x1,y1) ...

I'll add a constructor like this to Rectangle.

@smilingthax Do you have any test PDF files with forms that span the range of structures for AcroForm, such as a multi-level hierarchy, terminal fields with /Kids that contain multiple annotations, and non-trivial inheritance? If not, I will hand-generate some and test them with various readers to make sure I have the semantics right.

It will probably only take me half an hour to generate them, so no big deal if you don't. I am going to have to generate files with various types of errors in them anyway.

I'm pretty sure they will go in the doc helper because most of them will need to go there and, as you say, it's not good for the programmer to have to go to two places. Regarding the object handle knowing the QPDF it comes from, this is not true of direct objects that are created by the programmer until they are actually hooked in, so I don't want to ever rely on this.

Well, my line of thinking was: Every property of a form field that does imply changes to either pages/annotations or the (fields) tree structure, should not only be gettable, but also settable via the QPDFFormFieldObjectHelper -- even if this might mean that we have to somehow reference the /NeedAppearances key (which should probably be cached...) from the QPDFFormFieldObjectHelper, possibly by referencing the QPDFAcroFromDocumentHelper -- unless that would be too much of a burden ...
For all the other operation I fully agree that they have to be on the DocumentHelper.

A method to push inheritable things down may be useful at some point, but as you point out, we can't flatten the structure without potentially breaking fully qualified names.

What I had in mind was pushing down the fields (possibly only in the cache copy) without actually flatten the structure (i.e. different from what happens with the pages tree).

Right now, I am writing the code to assume that an annotation can be referenced by exactly one field, which is what the spec says. If an annotation is referenced by more than one field, I will warn and just keep one of them arbitrarily.

I'm not referring to annotations, which can only be associated with a single field, as your correctly said,  but to a field, which references more than one annotation in its /Kids array.
When one deletes such a field, and there is at least one annotations for it, these would either block the deletion, or that have to be deleted, too -- i.e. removed from their page's /Annots array (if they have an page -- I'm not sure whether we should allow annotations apart from a page they are on).
The underlying assumption here is: I would not allow (widget) annotations not associated with a form field at all (this does not generalize to non-widget annotations, though). All the annotation and appearance properties basically depend on the underlying field type. One could imagine re-associating annotations to a different form field, but only when the field type matches, but the user could still create a new annotation from the desired form field, or do such tricky operation using low-level QPDFObjectHandles.

I'll add a constructor like this to Rectangle.
... and some way to convert a Rectangle to a QPDFObjectHandle?
Because at some point I probably want to call (e.g.) QPDFObjectHandle::replaceKey(key, ...rect...) ...

Do you have any test PDF files with forms ...
No. I haven't actually seen such pdfs in the wild yet ... (but I haven't searched a lot, either).

PS: Your newly added insertRootFieldsForPage(QPDFPageObjectHelper, std::set<QPDFObjectHandle>&) method puzzles me a bit... Why the "root objects" and not the "terminal fields" ... and why set<QPDFObjectHandle> and not set<QPDFFormFieldObjectHelper> ... (things will probably become clearer with your next commits)

PS: Your newly added insertRootFieldsForPage(QPDFPageObjectHelper, std::set&) method puzzles me a bit... Why the "root objects" and not the "terminal fields" ... and why set and not set ... (things will probably become clearer with your next commits)

Page splitting loses form fields. I'd like to fix that, and this is exactly the information that will be needed to fix it. For example, qpdf --empty --pages file-with-forms.pdf -- out.pdf loses form functionality, and in order to know which root elements to preserve in the document's /AcroForm dictionary, I need this information. Simply creating /AcroForm and copying those objects to it should be sufficient.

Regarding your other comments, generally speaking, caches are evil. In my professional life, I have seen countless weeks wasted tracking down obscure bugs caused from caches. Caching stuff like /NeedAppearances is not worth it. It's very cheap to look that up every time. I think the qpdf library strikes a pretty decent balance between performance and maintainability. There are certainly extraneous memory allocations, function calls, and object copies, but the amount of time spent doing this stuff is most likely small in comparison to the file operations that are at the heart of working with PDF. Caches are suitable for cases where it's very expensive to compute things or where there's lots of derived information. Anyway, I'm pretty clear about where I will and won't use caches, and my default is to trade away performance in favor of simplicity where possible.
These are good thoughts about the setting interfaces, but I doubt I'm going to have time to implement anything comprehensive this week. In addition to establishing these APIs, there are some bugs to fix and a handful of unrelated enhancements. I'll get as far as I can get in another day or two of work, and that will be it for the current iteration. At the end of the week, I will likely release 8.1. At this point, I expect not to have to break any binary interfaces.
Regarding setting and higher level interfaces in general, while I agree that special methods for getting and setting fields would be valuable, given the realities of my ability to work on qpdf, I am strongly favoring code for things that are complicated to do. So operations that require updating /AcroForm and also /Annots, knowing when to change /NeedAppearances, potentially unmerging annotation dictionaries from field dictionaries, and those kinds of things are much higher priority than just providing methods that do nothing but call replaceKey. Pull requests are always considered, of course, but as you know, I'm pretty tight about qpdf's API and very conservative about doing things that increase maintainability. Though adding simple one-line methods is, of course, low risk and easy...but unless it's comprehensive, it doesn't actually save the user from having to call replaceKey....

Well, you have the page annotations, then:

Find (via the cache) the terminal form fields: getAnnotationsForPage + getFieldForAnnotation .(maybe a combined method might be useful in other places and more efficient?)
2a. Filter the form field tree to contain only those root + internal nodes that are on a path to one of the terminal form fields. Also filter the /Kids of each terminal field to contain only annotations from (one of) the pages in the (whole...) page set (...to be extracted).
2b. Alternatively, for each terminal field extract the path to the root and filter /Kids to only the annotations that are on the given page. Finally, for all pages, merge all the paths back together to create the new fields tree of the output pdf.

Just copying all the root nodes would be too much. At least all the referenced annotations on pages that are not copied have to be removed. Which in turn could lead to inner or terminal fields that could be pruned (but keeping them is rather harmless) ...
[All the root nodes would be enough, when we want all the pages... but then one would not need the page argument ...]
When merging pages from different pdfs, things become more difficult. Merging of two field trees has to happen; unfortunately the pdf specification has some restrictions on fields with the same field name (e.g. "In particular, field dictionaries with the same fully qualified field name must have the same field type, value and default value."). What this boils down to (disregarding special tricks/cases) is: When merging two trees with root nodes having the same name, either they are merged into one output node (only possible when the contents of the dictionaries match!), or one of the fields has to be renamed...

Yeah, handling all the document-level things like outlines, threads, forms, etc., in splitting and merging is pretty hard. I may not end up implementing that method until I actually need it.

I'll add a constructor like this to Rectangle.

... and some way to convert a Rectangle to a QPDFObjectHandle?
Because at some point I probably want to call (e.g.) QPDFObjectHandle::replaceKey(key, ...rect...) ...

Not sure you got my intention here... the quoting indentation ended up a bit misleading. As it does not make sense to overload each array/object setter (like replaceKey) that currently receives a QPDFObjectHandle to also recognize Rectangle, either the Rectangle class should contain a method to convert it to a QPDFObjectHandle (you probably dislike the conversion "operator QPDFObjectHandle", so let's be more explicit), or we add a QPDFObjectHandle::newFromRectangle, or something similar. All I'm saying is that a Rectangle that does not round-trip back to a PDF representation seems half baked...
Forget about getMediaBox/getCropBox/getTrimBox/getArtBox... these are convenience methods that could find a home on the newly invented QPDFPageObjectHelper, but there are definitely by far more important things to work on.

All I'm saying is that a Rectangle that does not round-trip back to a PDF representation seems half baked...

You're right, I didn't get that you were saying that. You are correct. I will make it possible to create an object handle from a rectangle, most likely QPDFObjectHandle::newArray(Rectangle), though your suggestion of QPDFObjectHandle::newFromRectangle(Rectangle) is also good. I'll add that too and have it just call the other one.

Pushed untested implementation of QPDFAcroFormDocumentHelper

Test fail because of coverage errors. I've started adding QTC coverage cases for things I need to remember to write tests for.

commenting out coverage until I'm ready to test so I don't get meaningless test failures

Pushed untested implementations of object helpers. I'm going to take a short break before I start writing some tests.

Re: QPDFAnnotationObjectHelper::getAppearanceStream(...bool on)

The names /On and /Off in section 8.4 (Annotations) of the pdf reference are only exemplary.
In fact 8.6.3 (Field types), under "Check Boxes", it says: "The appearance of the off state is optional but, if present, must be stored [...] under the name Off. The recommended (but not required) name for the on state is Yes".
So how does one know which name to use (a bool is certainly not enough)? -> "The V entry [...] holds a name object [...] which is used to select the appropriate appearance [...]".
Probably the method should take another string for this subname, as it already does for which ...

Re: QPDFFormFieldObjectHelper::getFullyQualifiedName

I think you have to concat until you reach the root (no more /Parent) even if on the path some /T are missing...

Re: QPDFAcroFormDocumentHelper.cc

this->m->cache_valid is never set to true?
in analyze(): if ((! acroform.isDictionary()) && (acroform.hasKey("/Fields")))  should probably be something like (!dictionary)||(!hasKey(...)) ?
"// All annotations should have been encountered by traversing /AcroForm"
-> No. All /Subtype /Widget annotations could be expected to be found from /AcroForm, but not the other types (like Links, Underlines, Movies, ...)
This raises another interesting question...: Will the non-AcroForm annotations (which could be present without /AcroForm even existing in the document catalog) have (in the future) their own DocumentHelper? Are QPDFAnnotationObjectHelper actually only really used for form-related (i.e. /Widget) annotations? I could imagine an AnnotationDocumentHelper, which allows queries like "getAllAnnotationsOfType" or "getPageForAnnotation" -- but then things become tricky when QPDFAnnotationObjectHelpers are "shared" by the AcroFromDocumentHelper (cache 1) and the AnnotationDocumentHelper (cache 2) ...
"// Treat the annotation as its own field."
"          this->m->annotation_to_field[og] =  QPDFFormFieldObjectHelper(annot);"
-> Well, considering that annot could be a pure annotation dictionary, not merged with a field dictionary, the resulting QPDFFormFieldObjectHelper then is a rather odd creature... OTOH, we can easily create such things by calling the public QPDFFormFieldObjectHelper(QPDFObjectHandle) -ctor with an object that certainly is not a form field - or even a dictionary at all ...
in traverseField(): "// [...] Otherwise, the annotation fields might be there￼ to be inherited by annotations below it."
-> I don't think this is possible... Inheritance is only defined for form dictionary keys; also non-terminal fields can only be pure field dictionaries (Table 8.69 Entries common to all field dictionaries, under /Kids).
You might want to output warnings when some of the checks like ! field.isIndirect() in traverseField() cause an early return...

Good comments. I already caught the cache_valid never being set mistake in my testing. I'll look through the other ones. Some of them are likely pointing out errors in my code, but keep in mind that there are millions of ways that people write non-compliant PDFs. I've gotten pretty good at anticipating the kinds of mistakes people make. I can almost guarantee that PDF creators will get wrong the stuff about which fields can be inherited and when to merge annotation and field dictionaries. I'm trying to anticipate some of the kinds of errors people will make. But I'll make sure my interpretations are not going to mess up on valid files. I'll push up what I have right now, which some tests coded but not yet invoked -- I'm invoking them manually at the moment. Then I will go through your comments item by item. Thanks.

The names /On and /Off in section 8.4 (Annotations) of the pdf reference are only exemplary. ...
My reading of the spec here is that /AS is to be used. I will change the method so that on is a string that defaults to the empty string. If not specified and there is a value in /AS, I will use that. Otherwise, I will not look for subkeys.

I think you have to concat until you reach the root (no more /Parent) even if on the path some /T are missing...

Yeah, I guess so. The spec is not clear about what to do if some levels are missing. I'm just going to skip missing levels including the "." rather than generating multiple "."'s. It's hard for me to see how this is going to matter for us, really.

in analyze(): if ((! acroform.isDictionary()) && (acroform.hasKey("/Fields")))  should probably be  ...

Yeah, I botched my parentheses. Good catch.

No. All /Subtype /Widget annotations could be expected to be found from /AcroForm, but not the other types

Right again. I modified the check to only find missing /Widget annotations. This whole bit of code is probably superfluous, but in the simple case of a merged field/annotation dictionary, I can well imagine that someone could write code that would just forget to include it in /AcroForm, and some reader or form field extractor out there could still work. Then someone will report a bug that some xyz software can handle their file but qpdf can't see the form field.

I'm not ready to think about how this code will work with other annotation types. I think I haven't done anything at this point to exclude other annotation types since all this special case logic about widget annotations is in the AcroForm document helper.

@smilingthax I believe I have addressed all your comments. I enhanced the comment in the block of code that's adding orphaned widget fields to make my intent a little more clear.

Actually I forgot the comment about the warnings for truncating when fields aren't indirect.

Now I have addressed all your comments.

When accessing an annotation dictionary, I would use QPDFAnnotationObjectHelper ... but that does not even have a method to look up a key in the inheritance chain. Generally speaking, an annotation dictionary does not have a /Parent key (when the key is there, it is only because of the merger with a field dictionary)... when the pdf contains non-terminal field dictionaries (i.e. esp.: containing /Kids) containing annotation keys ("merged dictionary"), then it certainly isn't because the erroneous pdf producer expects inheritance for annotations to happen. It even can't expect that a normal pdf viewer will even consider to try using both the merged dictionary and what it finds below /Kids -- which could be fields or annotations, or (violating the spec again) a mixture of fields and annotations, because all that would require non-trivial special case handling in the viewer...
Don't get me wrong, I'm not against trying to repair damaged pdfs. I just think that we should limit our attempts to "reasonable" things (like missing /Type in an appearance stream) and not blatant ignorance of defining principles (like: a field is either non-terminal, i.e. has only fields as children, or it is terminal, i.e. has no fields as children, but only annotations [...and, for #(kids)=1, the annotation dictionary could be merged into the field dictionary, ... and #(kids)=0 is actually not allowed, ... and ...]).

My reading of the spec here is that /AS is to be used. I will change the method so that on is a string that defaults to the empty string. If not specified and there is a value in /AS, I will use that.

I totally forgot about /AS ...  from Example 8.17 (Section 8.6) I derive: The distinction between /AS and /V seems to become important for radio buttons, where each radio of a group has one distinct "on" name like /MasterCard, /Visa, ..., and they all have /Off. One of them has /AS set to its "on" name, the others have /AS /Off. The /V is on the /Parent and has the name of the selected radio child...

Actually I forgot the comment about the warnings for truncating when fields aren't indirect.

That comment was meant as just one example of where a warning probably makes sense. Other places could be when encountering a loop via /Kids or /Parents, or when a dictionary is expected but the type does not match...
Another thought I had, which goes in a similar direction, was to make the constructors of (some of) the ObjectHelpers private, so that they can only be generated by the DocumentHelper, which in turns makes sure to only return ObjectHelpers pointing to sensible QPDFObjectHandles of the right type (or possibly null).

I'm not ready to think about how this code will work with other annotation types.

Ok.

I think I haven't done anything at this point to exclude other annotation types since all this special case logic about widget annotations is in the AcroForm document helper.

I just wanted to point out that we might end up with a problem in the future, when we have/need two DocumentHelpers (for normal annotations vs. AcroForm/widget annotations) that both try to cache information "attached to" QPDFAnnotationObjectHelpers, but these DocumentHelpers don't know of each other...

I don't think you've properly understood my reasoning on the repair. Most of your comments in the first paragraph do not apply to the code I've written. Unless I've misread the spec, there is no inheritance for annotations, and I have not written any inheritance for annotations. I didn't create extra accessors to look up arbitrary fields in the annotation dictionary because you can just use getObjectHandle().getKey(...). I don't know where you're getting the idea of annotation inheritance unless I misspoke in one of my earlier paragraphs. I am not supporting inheritance of annotation fields. I am supporting inheritance of fields in form field dictionaries that we don't know about. I am not blocking creation of one kind of helper with the wrong kind of underlying object.  I tried that with pages, and every kind of sanity check that a page was really a page backfired with at least one malformed file that would work fine if I just treated the thing as a page.
The purpose of my handling of "orphaned" widgets in this way is to specifically handle the case of a properly merged field/annotation dictionary that is simply missing from the /AcroForm tree. I don't think it's a blatant misunderstanding. I think it's an obvious error.
I don't wish to be dismissive of your comments as they are almost always good. I just think we are misunderstanding each other.
Do you see a specific place in the code where this code will misinterpret a correct file? Do you see any place in the code where adding additional checks will have any affect other than to prevent qpdf from processing some files that it might otherwise be able to process? It's entirely possible that I have either misread something in the spec or coded something incorrectly. I haven't done thorough testing yet, and I wrote the code pretty fast. Before I consider this done, I will have run various tests across various files, some from the wild and some of my own creation, and I can also review the sections of the spec while my implementation is fresh in my mind.

I just wanted to point out that we might end up with a problem in the future, when we have/need two DocumentHelpers (for normal annotations vs. AcroForm/widget annotations) that both try to cache information "attached to" QPDFAnnotationObjectHelpers, but these DocumentHelpers don't know of each other...

The AcroForm document helper is not an annotation helper. It is a form helper. Since forms are partially implemented with annotations, it uses the annotation object helper. If there is some need for some other kind of document helper that also needs to know about annotations, then it will also use the annotation helper. There is nothing in the annotation helper that knows that it is being used for forms. As such, I don't think I see the problem you are alluding to. If the annotation helper were polluted with logic specifically related to forms, then the problem you are concerned about would be a risk.

I'm tied up with other stuff tomorrow morning. I will be back at this by around 11:00 a.m. UTC-0:400. If you can present clear examples of where my API is wrong, I will adjust. Otherwise, I will proceed with testing, during which I may discover problems with my existing implementation. Once I have all the read-only stuff working, I will squash commits and, if time permits, maybe consider some of the modify stuff, but more likely, I will switch focus to the other qpdf-related issues I want to fix. Please let me know if you think these APIs will simplify any of either the form-flattening or banner code.

Please let me know if you think these APIs will simplify any of either the form-flattening or banner code.

I think this covers most of what Sahil needs, except for the modification part... and these operations should also force /NeedAppearances true... but I'm not sure how you would want to implement that...
It also looks pretty good wrt. to implementing (re-)generation of appearances.
Still, I'd like to hear your opinion on whether, and then how this operation could be integrated into qpdf in the future. Probably QPDFAcroFormDocumentHelper would be the right place... but where would you want to put the field-type specific (re-)generator implementation? Also where should this "add char widths", "convert text from field encoding to font encoding" and "parse the /DA string to obtain the Tf operators arguments"(your recent token-filter probably help here, too) go?
The actual flattening finally just has to, for each page, obtain /Annots and do its thing for each of the /Widget annotations in there (requiring only the annotation and the page dictionary; the AcroForm / fields tree is not needed here any more, as the field content was copied into the appearance stream [or /AS was set according to /V] of the annotation during the (re-)generation step).

Thanks for your clarification. I must have had a brief brain outage when I was coding that. It's not that much code. I'm just going to reread the relevant spec sections and the reread through the code. I think that, at the beginning, the distinction between fields and annotations was murky in my mind, though it is very clear now. Some of the first bits of code probably reflected this murkiness.
Your help with this is very valuable. You're really the only other person who has studied the code in any depth and provided useful input at this level. When I'm back online later today, I will address the above issues. After sleeping on it, I think you're right that efforts to repair are probably not as useful as I was thinking. Once I actually coded analyze, it was simpler than I originally had in my mind. My main concern was just that, when iterating through /Annots and finding widget annotations, I always wanted to be able to map them back to fields. Anyway, there's nothing like a good night's sleep, so I'll take a pass through the code again later this morning when I'm back from my appointments.

I am not blocking creation of one kind of helper with the wrong kind of underlying object.

Yes, but you also won't do any check for the right type in non-trivial methods. IIRC Pranjal once did some operations on an QPDFObjectHandle without checking its type, and got an exception/segfault, which he didn't understand. With an ...ObjectHelper as further wrapping around the QPDFObjectHandle, it would be even more difficult for the programmer to understand why his code / his use of qpdf then fails only for certain pdfs. In node-libqpdf I specifically added assertInitialized() to many QPDFObjectHandle method wrappers to obtain exceptions (which I would convert to JS exceptions) instead of the whole VM crashing with a segfault...

Re: QPDFAcroFormDocumentHelper::getAnnotationsForPage
In its current form, this method is not specific to AcroForms, although it is part of the AcroFormDocumentHelper interface... Now, I'm not sure whether we will ever need a full AnnotationsDocumentHelper... But what could be helpful specifically for AcroForms would be: getWidgetAnnotationsForPage.
The next realization: Conceptually this would only be a specialization of getAnnotationsOfSubtypeForPage(page, subtype)...
Idea: vector<QPDFAnnotationObjectHelper> QPDFPageObjectHelper::getAnnotations() Possibly with overload ...PageObjectHelper::getAnnotations(const string &subtype), and/or AcroForm::getWidgetAnnotationsForPage as special case for /Subtype /Widget ...

In node-libqpdf I specifically added assertInitialized() to many QPDFObjectHandle method wrappers to obtain exceptions (which I would convert to JS exceptions) instead of the whole VM crashing with a segfault...

In qpdf 8, I replaced almost all cases where qpdf called assert and replaced it with calls to various exceptions or with sensible defaults. It's a very tough call, and it's one thing I would change if I could change it in a reasonable way....one has to strike a balance between helping programmers that use the library vs. allowing qpdf to handle broken files. This is your classic static vs. dynamic typing. The annoyance with QPDFObjectHandle is very much like programming in groovy or perl where treating something as a type that it isn't results in a runtime error. At one point, I actually considered changing the API to make it much harder to do this by allowing people pass defaults when querying objects and/or by giving warnings. The problem is that QPDFObjectHandles and the underlying objects can exist in a vacuum, and also, making those kinds of changes would require radical rewrite of parts of the library. I'm afraid it's a weakness we have to live with because there's no way I am going to have time to improve it much beyond its current state. However, at least in qpdf 8, any QPDFObjectHandle created through parsing a file contains enough context that it can be used in a good warning. Most of the time, if you access on object as something of the wrong type, you get both a warning and a sensible default. All that said, there's only so much a library can do to protect against programmer errors. The qpdf library does require a higher level of knowledge of PDF than, say, poppler, itext, pdfbox, or similar libraries that try to abstract away knowledge of the PDF spec. All that said, your comments are valid, and I will take another pass though code with them in mind and see what I can tighten up. At least qpdf should make it easier for a programmer to notice if they are accidentally doing the wrong thing.

I like the idea of a specialized method to get all annotations of a particular subtype. I will add that.

getAnnotationsForPage belongs in page document helper. I will move it there, add a specialized version, and change the one in the form helper to specifically request widget annotations.

I'm getting rid of the code that adds missing parents.

@smilingthax I've pushed up more fixes. Does this feel better to you? Take a look at the specific commit 4bb0f2d66c2c8938846bd5b1b12771eac5c5cc04 if you want. Once you've looked at it, I'll squash with the other commits. I think this should help with some of your objections, and I think it simplifies and improves the behavior as well.

pushed up a fix to a typo in a comment

Fixed typos

Nothing catches my eye after the recent changes.
There are some things in my head when it comes to modifying the structures (adding new fields, etc), but that's for another time.

Thanks again. As I said, this is better for your review and participation. I will squash these and work on testing, then I'll assess about the modifications.

Regarding write functionality, I can think of many possibilities around manipulation of the fields tree and so forth, but I think these two basic ones would be pretty trivial to add:

Create a new object to contain a merged field/widget annotation; populate it with /Subtype /Widget, and hook it into the containing pages' /Annots field and the documents /AcroForm field, creating each as needed
Modify the value of /NeedAppearances
Set the value (/V) of a widget annotation while also setting /NeedAppearances true
I think these modifications would be sufficient to handle most of bannertopdf.

One additional thing that could help would some assistance in generating appearance streams, such as taking the value of /DA, doing the marked text substitution, etc. -- the stuff the lexical changes were added to support. I don't have that fresh in my mind, but read the relevant spec sections a few weeks ago and manually edited a few appearance streams in qdf files to convince myself that I understood how they worked.
Longer term, I could adding support for splitting merged field/annotation dictionaries (though this requires a lot more knowledge about keys than I think I want to deal with) and doing other manipulations on the fields tree, but I suspect other things will be of higher value for my limited available time.
Of course, support for flattening forms could also be added, though I still think it makes sense to get that stuff working outside the library first. Doing it in the general case is going to be very difficult once you deal with XFA, older formats with boundaries, and all sorts of other nuances. I have a feeling the poppler/ghostscript approach is going to be the approach for a while. But I think the existing changes will make it easier to write that.
I'd be interested to know if you have additional thoughts. I will probably distill them into a section in qpdf's TODO file in case I can't get back to it for a long time. Depending on how the next couple of days go, I could conceivably do the first set of bullets this week. We'll see.

I know you mentioned other stuff about adding and deleting fields in earlier discussions.

Possibly creating a field with child annotations would also be doable. This seems to be required for radio buttons.

Setting /V (incl. /NeedAppearances) is probably the most useful right now.

"[...] some assistance in generating appearance streams [...]"... I don't feel like I currently can give a lot of input on that, without having to dig through that part of the spec again (and/or trying to implement parts of the process). BTW: XFA appearance streams have to be generated by the viewer application that makes changes to the form (and thus is already capable of handling rich text), so no code needed for that.

Form flattening then simply means: Adding an image draw command with the appearance stream as source (they are actually /XObjects, although the  /Subtype seems to be missing in some pdfs -> must be added by us, when missing!) to the page's content stream. And there's a bit of bookkeeping involved, like calculating+setting the correct context matrix for the drawing command (depends e.g. on /Rect and the Flags), adding it to the resource dictionary without a key collisions (the /XObject can have the expected key also stored as /Name, which has to also be changed). All in all nothing exciting, but will require some time for testing, etc.

I'm not sure what you're alluding to in "older formats with boundaries"...

But I don't think that any special cases for XFA are needed here.

It's just that you can't flatten without having up-to-date appearance streams first.

"Longer term, I could adding support for splitting merged field/annotation dictionaries (though this requires a lot more knowledge about keys than I think I want to deal with) and doing other manipulations on the fields tree"

Hmm... IMO that's where it actually gets interesting.... I think that the library cannot (and should not) shield the user from understanding the difference between an annotation (dictionary) and a form field (dictionary). Therefore I'm not to keen on providing a high-level "create a new field with annotation" method, together with some special handling of radio buttons... What the library can do is relieve him from the duty of having to deal with merged vs. non-merged dictionaries. Some Ideas:

bool QPDFFormFieldObjectHelper::isTerminal()
void QPDFAcroFormDocumentHelper::addAnnotationToField(QPDFFormFieldObjectHelper &field, QPDFAnnotationObjectHelper &annot)
-> this is only possible for field->isTerminal()
-> must split the field/annotation dictionary, when it is merged
-> Where does the new QPDFAnnotationObjectHelper come from? Does the user have to create a QPDFObjectHandle from scratch and convert it to a
-> sanity check that the annotation is not used for two fields...
-> third argument to add annotation to page for convenience? (via method in QPDFPageDocumentHelper::addAnnotation(page,annot) that makes sure that the annotations is not used twice [problem: we don't have a cache for that right now, esp. not in the PageDocumentHelper...])
void QPDFAcroFormDocumentHelper::removeAnnotationFromField(...)
-> but still part of the page (? - convenience: , bool removeFromPage? [Problem: we don't know the page the annotation is on - nobody does... -> instead of putting addAnnotation/removeAnnotation in the PageDocumentHelper, an AnnotationDocumentHelper seems to be the appropriate place...])
-> split of dictionaries might be necessary here, as well
-> How to deal with nodes and paths in the fields tree that have no Kids? - the pdf spec does not want them to exist, and actually they are neither terminal nor non-terminal ...
void QPDFAcroFormDocumentHelper::addField(QPDFFormFieldObjectHelper &field, QPDFFormFieldObjectHelper *parent = null)
-> parent==null: add to root (/AcroForm's /Fields array)
-> only for !parent->isTerminal()
void QPDFAcroFormDocumentHelper::removeField(QPDFFormFieldObjectHelper &field)
-> has to somehow deal with the /Kids, which could be just annotations (remove? split field/annotation dictionary?), or even a whole subtree of fields+annotations...

=> This is probably too much to implement right now. But the gist is: A method to split a field/annotation dictionary is the low-level primitive required here... as we (currently) do not know (without brute force) the page an annotation is on (to make changes to /Annots), the annotation dictionary must keep the existing objId, and the split off field dictionary is a new object. The method also has to fix the links (/Parent, /Kids). Merging, on the other hand, is entirely optional - everything will still work when we don't implement it and generate one-element /Kids.
Regarding the need to hard-code a list of all the possible keys ... it's not too bad, actually:

Annotation of type Widget: /Type, /Subtype, /Rect, /Contents, /P, /NM, /M, /F, /AP, /AS, /Border, /C, /StructParent, /OC, /H /MK, /A, /AA, /BS
-> easy way out: everything not in this list is moved to the field dictionary
Field (all possible /FT types): /FT, /Parent, /Kids, /T, /TU, /TM, /Ff, /V, /DV, /AA [!! must become part of both dictionaries !!], /DA, /Q, /DS, /RV, /Opt, /MaxLen, /TI, /I, /Lock, /SV

The safe way to treat unknown keys would be to simply duplicate them into both dictionaries (as needed for /AA).
Actually the splitting could become part of the existing getFieldForAnnotation method:
QPDFFormFieldObjectHelper getFieldForAnnotation(QPDFAnnotationObjectHelper annot, bool separate=false)
When separate==true the method makes sure that annot and the returned field are split, when they are not already.

Thanks for your comments. Yeah, looks like we're pretty much on the same page. Your idea of duplicating keys we don't know whether are part of the field or annotation is a good idea. I hadn't thought of that, but maybe I would have if I actually started to code it. :-)
The generating appearance stream thing is basically using /DA and /V to construct an appearance stream that displays the text in /V. It's pretty straightforward for cases where /V can be represented with characters in the standard fonts and the field is a text field. For other kinds of fields, the appearance stream is more complicated. Regarding boundaries, if I recall the spec correctly, prior to the introduction of appearance streams, there were fields controlling how boundaries looked around rectangles and such. I think they are safe to ignore with newer readers, and I think I would just ignore them. The complication of generating appearance streams comes when there are characters that require additional fonts, as we have already discussed. Basically /V can be a Unicode string, but the string in the appearance stream has to be properly encoded/character mapped for the font, and writing that code is way out of scope.
At the end of all this, whatever I don't get to, I will copy your and my comments into a new issue and make a note in the TODO file that all previous thoughts are gathered into that issue.
I'm most of the way through testing and have hand-constructed files with non-trivial inheritance and a few other details. There are some bugs in atril where it doesn't render some fields properly that the PDF viewer in chrome does. I'm not surprised though....I ran into a problem in my day job (which doesn't ordinarily have anything to do with PDF, but I tend to get called upon when someone in the company has a PDF problem!) regarding Arabic values in forms. So many different readers all broke in different ways, as did various Java PDF libraries. Support for forms just isn't that good, I guess.

Hmm, one other feature that comes to my mind (from pdftopdf, and will be necessary for form flattening, again):
When I add commands to a page's content stream, I have to make sure the graphics state is in a "known" state, ideally: the initial state. While this is trivial for prepended content, the trick to make this work for appended content is to make sure that everything before is properly wrapped in q/Q (push/pop graphics state).
Now, the "safe" option is to prepend a q and append a Q for before appending my new content. The graphics stack size is limited, though.
So a better idea would be to have a method in QPDF that uses your recent lexical additions to walk the existing content stream and checks whether any operators (or, more exactly: any graphics state changing operators - but a conservative approximation is certainly good enough) appears a the "top level" (i.e. outside of a q/Q pair). In that case the whole existing content stream has to be wrapped by prepending q/appending Q (as I current do)...

Tests are passing. There were a few minor problems, but nothing significant. I have to stop for the day since I have rehearsal tonight, but when I resume, I will update ChangeLogs and documentation and most likely merge this increment, though until I do an actual release, I still reserve the right to change it. Or I may hold off on merge just a bit to give @sahilarora535 a little longer to look at it. I will probably proceed with the other non-form-related fixes before I move onto writing.

I ended up deciding to just add a basic method to set the value of a field, supporting Unicode encoding and modifying /NeedAppearances. I felt that this was a small amount of work that I could do easily while it's fresh in my mind, and it would take care of a lot of what is needed for bannertopdf. I will push up my comments in a few minutes.

Best way to look at latest changes is just to look at the last few commits, especially "Allow setting a form field's value"

I added an example, updated docs, ChangeLog, etc. I am now moving onto the next item.


Aliases in .launchdarkly/coderefs.yaml not working

I am attempting to setup this action but it looks like it is not correctly referencing the .launchdarkly/coderefs.yaml file as it should, noted in the README.
We have another action in the same repo running the<EMAIL_ADDRESS>action, which does correctly find code alias references, so the config yaml should be working and referenced correctly.
.launchdarkly/coderefs.yaml
projects:
    - key: platform
      dir: src
      aliases:
        - type: camelcase

code-refs-in-pr action
Here's the relevant section of the job from the workflow file
  launch-darkly-pr-diff:
    runs-on: ubuntu-latest
    name: Find LaunchDarkly feature flags in diff
    steps:
      - name: Checkout
        uses: actions/checkout@v4
      - name: Find flags
        if: "!contains(github.event.issue.labels.*.name, 'maintenance')"
        uses: launchdarkly/find-code-references-in-pull-request@v2
        id: find-flags
        with:
          project-key: platform
          access-token: ${{ secrets.LAUNCH_DARKLY_CODE_REFS_SECRET }}
          repo-token: ${{ secrets.GITHUB_TOKEN }}

action running launchdarkly/find-code-references-in-pull-request@v2
I have changed/removed some key names and references, but in the logs it lists empty lists aliases for all the flag keys. I put one actual kebab case flag reference in the source to make sure that the action and comments were working as intended and to debug whether it was an aliasing issue. The kebab key reference that I put in is found and a comment is left correctly, but the other flag references are never noted. Note the aliases are empty lists in the debug log as well.
 ##[debug]Getting pull request diff...
  ##[debug]Got 6 diff files
  2024/07/09 16:29:42 Generating aliases...
  2024/07/09 16:29:42 Finished generating aliases...
  ##[debug]Generating aliases for removed files...
  ##[debug]Generated aliases for 'example-flag-name':  []
... <repeated for all flags, empty list values for all>
...
  2024/07/09 16:29:43 Searching for 620 flags
  2024/07/09 16:29:43 Summarizing results
  ::endgroup::
::group::Scanning diff for references...
Scanning diff for references...
##[debug]Setting outputs...
##[debug]setting output any-modified=true
##[debug]setting output modified-flags-count=1
##[debug]setting output modified-flags=test-kebab-name

Existing action  running<EMAIL_ADDRESS>In the existing action you can see that it does find code references correctly aliased ("4 code references across 620 flags and 2 files")
INFO: 2024/07/09 17:35:08 coderefs.go:27: absolute directory path: /github/workspace
INFO: 2024/07/09 17:35:08 git.go:48: git branch: ld-flag-actions
INFO: 2024/07/09 17:35:12 coderefs.go:148: sending 4 code references across 620 flags and 2 files to LaunchDarkly for project(s): [ platform]
INFO: 2024/07/09 17:35:12 coderefs.go:180: checking if 617 flags without references were removed in the last 10 commits for project: platform

Please let me know if there's any other information I can provide. Thanks!

Hello, just wondering if there's any update or estimate on this? Thanks!

Hi @DigTheDoug apologies for the delay here, and thanks for your patience on this issue!
I've been able to reproduce this and found that there's an issue with setting aliases on the project level. We are looking into fixing this but in the meantime, a quick workaround would be to set the aliases on the global level as well. So something like:
aliases:
  - type: camelcase
projects:
    - key: platform
      dir: src
      aliases:
        - type: camelcase

Let me know if this workaround works for you, thanks!

Thanks for the response. I can confirm that using the aliases key at the global level does work for cases where we only have one project in the repo. We don't have monorepos, but unfortunately all of our repos do use that format with the projects key, so I will see if we can change them to be the non-nested version. Thanks!

Thanks for the response. I can confirm that using the aliases key at the global level does work for cases where we only have one project in the repo. We don't have monorepos, but unfortunately all of our repos do use that format with the projects key, so I will see if we can change them to be the non-nested version. Thanks!

Great - glad to hear it works for you! If you find this approach doesn't work for one of your repos, let us know and we'll see what we can do. It may be some time before we can add monorepos/multi-project support to this action.
I'll go ahead and close this issue out, but feel free to reopen it if you have any more questions!


feature: 单据审批新增事物控制

Codecov Report

Merging #662 (8c1acc7) into v2.6.x_develop (92d85d4) will decrease coverage by 0.32%.
The diff coverage is 62.12%.

@@                Coverage Diff                 @@
##           v2.6.x_develop     #662      +/-   ##
==================================================
- Coverage           62.26%   61.94%   -0.33%
==================================================
  Files                 442      459      +17
  Lines               27470    27967     +497
==================================================
+ Hits                17104    17323     +219
- Misses              10366    10644     +278

Impacted Files
Coverage Δ

itsm/ticket/views/ticket.py
46.58% <0.00%> (-0.07%)
:arrow_down:

itsm/openapi/decorators.py
57.69% <57.69%> (ø)

itsm/trigger/models/action.py
46.84% <60.00%> (ø)

requests_tracker/signals/handlers.py
70.00% <75.00%> (-2.42%)
:arrow_down:

itsm/openapi/service/views.py
51.85% <100.00%> (+3.33%)
:arrow_up:

itsm/openapi/ticket/views.py
46.07% <100.00%> (-0.06%)
:arrow_down:

itsm/component/utils/lock.py
84.84% <0.00%> (-9.60%)
:arrow_down:

itsm/monitor/views.py
75.00% <0.00%> (-5.00%)
:arrow_down:

urls.py
100.00% <0.00%> (ø)

itsm/openapi/base_service/views/transition.py
57.50% <0.00%> (ø)

... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb502be...8c1acc7. Read the comment docs.


Feature: Pass tags to s3 buckets in s3-archive module

Since we control a lot of our infrastructure via tags, it is very important for us to be able to set them explicitly. Unfortunately, in the current version of the s3-archive module it is not possible to pass tags, which are then set for the metrics and log buckets.

@HansBraun
Is it important for you to tag the two s3 buckets differently? We have several of our TF modules with support for "global" tags, where tags will be applied to all created resources, identically. We easily add that same support to this module. But, it sounds like you need individual tags though, per resource. Is that correct?

@HansBraun
Thanks for your update. While investigating this issue, I found that the AWS Provider module already provides support for tagging of all elements with a default tag as detailed here:
https://registry.terraform.io/providers/hashicorp/aws/latest/docs/guides/resource-tagging#propagating-tags-to-all-resources
It looks like adding support for this use case is no longer necessary, as it's available at the provider level.

@HansBraun
Does the AWS Provider tagging route work for you? Were you able to get that working for your use case?

@MichaelBriggs-Coralogix  Yes, it's working in my use case, as I am fine with tagging both buckets equally.


Scan Barcode Using zxing Within WebView Android

how to passing back data to android web view from barcode scanner zxing?

cookies are currently our only solution to share data between webview and the webpage.


Lines not delimited by CRLF sequence

I'm trying to provide a calendar feed of multiple events for a calendar client to subscribe to:
let calendar
let count = 0

bookings.forEach(booking => {
    let event = new ICalendar({
        title: `Paw Paddock ${booking.paddock.name}`,
        start: app.dateNew.fromSQL(booking.startAt).toJSDate(),
        end: app.dateNew.fromSQL(booking.endAt).toJSDate(),
        location: `Paw Paddock ${booking.paddock.name}, ${booking.paddock.fullAddress}`,
        description: `${booking.sessionLength} minutes at Paw Paddock ${booking.paddock.name}`
    })

    if (count === 0) {
        calendar = event
    } else {
        calendar.addEvent(event)
    }

    count++
})

reply
    .code(200)
    .headers({
        'Content-Type': 'text/calendar; charset=utf-8',
        'Cache-Control': 'max-age=0',
        Pragma: 'no-cache'
    })
    .send(icalString)

Which produces something like this:
BEGIN:VCALENDAR
VERSION:2.0
BEGIN:VEVENT
CLASS:PUBLIC
DESCRIPTION:25 minutes at Paw Paddock Andover
LOCATION:Paw Paddock Andover\, Longparish Rd\, Andover SP11 6NL
SUMMARY:Paw Paddock Andover
TRANSP:TRANSPARENT
DTSTART:20230416T090000Z
DTEND:20230416T093000Z
UID:bijua0u7ry7
DTSTAMP:20230308T230302Z
END:VEVENT
BEGIN:VEVENT
CLASS:PUBLIC
DESCRIPTION:25 minutes at Paw Paddock Andover
LOCATION:Paw Paddock Andover\, Longparish Rd\, Andover SP11 6NL
SUMMARY:Paw Paddock Andover
TRANSP:TRANSPARENT
DTSTART:20230416T080000Z
DTEND:20230416T083000Z
UID:0r8sufd8edf
DTSTAMP:20230308T230302Z
END:VEVENT
PRODID:datebook
END:VCALENDAR

However it fails to import into iCal as a URL subscription. I ran the file through a validator: https://icalendar.org/validator.html
and it came back with:

It does however work in iCal if I manually import the download file rather than using the URL.

This was a non issue. It didn’t like my local url. Worked fine from remote server


Set framework to v4.5 to work with MonoDevelop.Addin 0.3.0

Setting framework within proj file to override default framework within MonoDevelop.Addin 0.3.0
Ref #31

Oh thanks for figuring that out! Fixes #31


Add admonition blocks

This PR adds the ability to easily create so-called admonition blocks, which look like this:

The "warning" and "info" blocks were introduced in PR #7. This PR makes a few modifications and adds new blocks.

This one needs a rebase now

@dmcgowan Done

usually it's because someone uses cut and paste to sign and does not separate the sig line from the subject line..

What's the appropriate course of action here? I haven't yet run into this.

 But, if you have a stack of commits, I find the rebase -i capability helpful to go fix up commits not at the "end"

hmm I pulled down his pr and .. well I don't see anything wrong with his commit signature

yeah the broke one is the first one.. not sure why travis marked the last one..

so I think what happened is instead of rebasing.. he created a new commit and in the middle we added the new test for dco.. so boom

mike@mike-VirtualBox:~/go/src/github.com/containerd/containerd.io$ sudo git rebase master
First, rewinding head to replay your work on top of it...
Applying: Add remaning admonition blocks (requirement, danger, success, etc.) plus README documentation
Using index info to reconstruct a base tree...
M	README.md
Falling back to patching base and 3-way merge...
Auto-merging README.md
CONFLICT (content): Merge conflict in README.md
error: Failed to merge in the changes.
Patch failed at 0001 Add remaning admonition blocks (requirement, danger, success, etc.) plus README documentation
The copy of the patch that failed is found in: .git/rebase-apply/patch

When you have resolved this problem, run "git rebase --continue".
If you prefer to skip this patch, run "git rebase --skip" instead.
To check out the original branch and stop rebasing, run "git rebase --abort".

so at this stage you open up your fav editor and fix the conflicts.. save the fixed file then
mike@mike-VirtualBox:~/go/src/github.com/containerd/containerd.io$ sudo git status
rebase in progress; onto 067f3fa
You are currently rebasing branch 'lperkins/admonitions' on '067f3fa'.
  (fix conflicts and then run "git rebase --continue")
  (use "git rebase --skip" to skip this patch)
  (use "git rebase --abort" to check out the original branch)

Changes to be committed:
  (use "git reset HEAD <file>..." to unstage)

	modified:   assets/sass/style.sass
	modified:   layouts/partials/admonition.html
	new file:   layouts/shortcodes/danger.html
	modified:   layouts/shortcodes/info.html
	new file:   layouts/shortcodes/requirement.html
	new file:   layouts/shortcodes/success.html
	modified:   layouts/shortcodes/warning.html
	modified:   resources/_gen/assets/sass/sass/style.sass_6af550c30182af2cbd2f775fdbb9d3bc.content
	modified:   resources/_gen/assets/sass/sass/style.sass_6af550c30182af2cbd2f775fdbb9d3bc.json

Unmerged paths:
  (use "git reset HEAD <file>..." to unstage)
  (use "git add <file>..." to mark resolution)

	both modified:   README.md

mike@mike-VirtualBox:~/go/src/github.com/containerd/containerd.io$ sudo git add README.md
mike@mike-VirtualBox:~/go/src/github.com/containerd/containerd.io$ sudo git status
rebase in progress; onto 067f3fa
You are currently rebasing branch 'lperkins/admonitions' on '067f3fa'.
  (all conflicts fixed: run "git rebase --continue")

Changes to be committed:
  (use "git reset HEAD <file>..." to unstage)

	modified:   README.md
	modified:   assets/sass/style.sass
	modified:   layouts/partials/admonition.html
	new file:   layouts/shortcodes/danger.html
	modified:   layouts/shortcodes/info.html
	new file:   layouts/shortcodes/requirement.html
	new file:   layouts/shortcodes/success.html
	modified:   layouts/shortcodes/warning.html
	modified:   resources/_gen/assets/sass/sass/style.sass_6af550c30182af2cbd2f775fdbb9d3bc.content
	modified:   resources/_gen/assets/sass/sass/style.sass_6af550c30182af2cbd2f775fdbb9d3bc.json

mike@mike-VirtualBox:~/go/src/github.com/containerd/containerd.io$ sudo git rebase --continue
Applying: Add remaning admonition blocks (requirement, danger, success, etc.) plus README documentation
mike@mike-VirtualBox:~/go/src/github.com/containerd/containerd.io$ sudo git status
On branch lperkins/admonitions
nothing to commit, working directory clean

then from there fix the commit header that's too long via:
mike@mike-VirtualBox:~/go/src/github.com/containerd/containerd.io$ sudo git commit --amend
[lperkins/admonitions 90d060b] Add remaning admonition blocks (requirement, danger, success, etc.) plus README
 Author: lucperkins<EMAIL_ADDRESS> Date: Thu Oct 18 10:24:21 2018 -0700
 10 files changed, 44 insertions(+), 11 deletions(-)
 create mode 100644 layouts/shortcodes/danger.html
 create mode 100644 layouts/shortcodes/requirement.html
 create mode 100644 layouts/shortcodes/success.html

node just need to delete the last word there.. then
$ git push -f origin branch
hope this helps cheers

@mikebrow Thanks a lot for your help! This should be all squared away now.


Include partials in views

This avoids repeating code in the different views

Nice refactoring, will test it out today before merging.

Ok, no problem @marktopper  :+1:

Ok, no problem :+1:

Ok, no problem :+1:


Modernize tool

Use SHA-256 instead of MD5 to shorten file names (doesn't matter but good hygiene)
INFRA-2594: Add support for new Gradle versions
Remove stray top-level permissions file
General dependency modernization, subsumes #1447

Someone will complain if permissions are gone 🤷


Support of Pinterest API (v5)

Related to the previously reported issue #122, Pinterest has published a brand new API and deprecated the previous API.
https://developers.pinterest.com/docs/api/v5/
For instance, to exchange the code for an access token: https://api.pinterest.com/v5/oauth/token
Do you have plans to support the new API?

I haven't been actively maintaining this project recently, but never say never :wink: Thanks for the notice though and definitely feel free to submit a PR yourself.

Thanks in advance for all your efforts @dirkgroenen
To prepare a PR, it looks like all entry points have changed. Does it have to be a completely new version of the library?

We can probably release it as a new major, with clear documentation in the README.md informing about the difference between 0.x.x and 1.x.x.


开启sky-php-agent后elasticsearch7 查询报错

Describe the bug
A clear and concise description of what the bug is.
To Reproduce
Steps to reproduce the behavior:

Go to '...'
Click on '....'
Scroll down to '....'
See error

Expected behavior
A clear and concise description of what you expected to happen.
Screenshots
If applicable, add screenshots to help explain your problem.
PHP (please complete the following information):

OS: ubuntu
Browser [e.g. chrome, safari]
Version 7.2.31
PHP ini

SkyWalking (please complete the following information):

OS: 官方docker
Browser [e.g. stock browser, safari]
Version [e.g. 6.4]

Additional context
Add any other context about the problem here.
开启sky-php-agent
使用ruflin/elastica 7x的拓展查询会报
Content-Type header [application/x-www-form-urlencoded] is not supported
的Http请求错误，关闭sky-php-agent后正常

感谢反馈


Report not working on CI

I am testing around to see if I can integrate the playwright-slack-report to CI. However, it does not work.
The report section of my playwright config
 reporter: [
    process.env.CI ?
    [
      './node_modules/playwright-slack-report/dist/src/SlackReporter.js',
      {
        slackWebHookUrl: 'https://hooks.slack.com/services/xxxxxxxxx/xxxxxxxxx',
        channels: ['qa-api-test'], // provide one or more Slack channels
        sendResults: 'always', // "always" , "on-failure", "off"
        maxNumberOfFailuresToShow: 4,
        layout: generateCustomLayout,
      },
    ]:
    ['list'],
    ['html'],
  ],

When I use the report on local runs, I get the slack notification:
 reporter: [
     [
          './node_modules/playwright-slack-report/dist/src/SlackReporter.js',
          {
            slackWebHookUrl: 'https://hooks.slack.com/services/xxxxxxxxxxxxx/xxxxxxxxxxxxx',
            channels: ['qa-api-testing'], // provide one or more Slack channels
            sendResults: 'always', // "always" , "on-failure", "off"
            maxNumberOfFailuresToShow: 4,
            layout: generateCustomLayout,
          },
        ],
    ['html'],
  ],

Or maybe I am doing something wrong.
Previously, I had something like this;
export default defineConfig({
  // Concise 'dot' for CI, default 'list' when running locally
  reporter: process.env.CI ? 'dot' : 'list',
});

And I was using slack-notify Github Action which is working fine but I need to include the failing tests into the report which I could not with the slack-notify.

hey @galadim1 , can you tell me what error you are getting?

I was not getting any errors. The process.env.CI ? was not been triggered on CI:
The issue was, ci wasn't installed (I am now installing it inside the workflow).
In my yml file I now have:
yarn add ci --ignore-workspace-root-check
yarn add playwright-slack-report -D --ignore-workspace-root-check

Thanks, my issue is resolved.


rework validator page and node page

fix #339

[x] 1. change  the query method for validators.
[x] 2. delete the pagination for node page and validator page.
[x] 3. change page into validators page and online nodes page.
[x] 4. change dash board node explanation to total validating nodes and total online nodes.
[x] 5. add proposal table and display kickout nodes table.
[ ] 6. add test for new components

this PR is not finished, why do you approve now? need at least add kickout and proposal table in the node page. @frol

@icerove I suggest we add those extra pages in separate PRs. WDYT?

I am sorry, I do not read your last comment and already add  proposal table to it.

for auto refresh nodes, I want to make a separate PR to work.


 RtEpochs subclasses _BaseEpochs, and most other classes like EpochsArray and the upcoming EpochsKit will subclass Epochs because Epochs has a ton of functions that _BaseEpochs has. I propose that Epochs, EpochsArray, and EpochsKit should all subclass _BaseEpochs, and Epochs should only have the bits of code necessary to load and save the FIF version of Epochs. (This would make Epochs a very small class.) It would separate the FIF-specific functionality from the generally useful baselining/rejecting/etc. functionality.
The problem is that we need to take care of what methods RtEpochs cannot handle from the improved base class _BaseEpochs, which might take some thinking.

It's probably better to wait until after 0.9 release to do this, so that we can gradually discover any subtle bugs that we introduce from these changes instead of including them in a release.
This should also be coupled with a documentation of what all you can do with Epochs, possibly in table format: preload True/False, proj=True/False/delayed, etc.

+1


health_monitor doesn't report index during bosh recreate

Recreation Steps:

Deploy bosh or bosh lite with bosh release 262.4

Deploy vms from a release (e.g. loggregator)

ssh onto director

Create a fake JSON plugin receiver
sudo -i

mkdir -p /var/vcap/jobs/director/bin/bosh-monitor

cd /var/vcap/jobs/director/bin/bosh-monitor

cat << EOL >> run.sh
#!/bin/bash
cat - >> /tmp/json-plugin.log
EOL

chmod +x run.sh

monit restart health_monitor

tail -f /tmp/json-plugin.log

In a separate terminal, bosh recreate <vm>

Expected Output:
{"kind":"heartbeat","id":"fd5ded9b-9cbc-4f51-a1ed-363fa77bfc1b","timestamp":1507655859,"deployment":"loggregator","agent_id":"f6d0703a-48ae-41b0-aefc-8305c12f4a2d","job":"doppler","index":"","instance_id":"eca69415-6770-467f-a0aa-6cb620a0b6d0","job_state":"running","vitals":{"cpu":{"sys":"7.0","user":"22.1","wait":"0.8"},"disk":{"ephemeral":{"inode_percent":"2","percent":"3"},"system":{"inode_percent":"14","percent":"27"}},"load":["0.45","1.05","0.97"],"mem":{"kb":"1502632","percent":"37"},"swap":{"kb":"4944","percent":"0"}},"teams":[],"metrics":[{"name":"system.load.1m","value":"0.45","timestamp":1507655859,"tags":{"job":"doppler","index":"","id":"eca69415-6770-467f-a0aa-6cb620a0b6d0"}},{"name":"system.cpu.user","value":"22.1","timestamp":1507655859,"tags":{"job":"doppler","index":"","id":"eca69415-6770-467f-a0aa-6cb620a0b6d0"}},{"name":"system.cpu.sys","value":"7.0","timestamp":1507655859,"tags":{"job":"doppler","index":"","id":"eca69415-6770-467f-a0aa-6cb620a0b6d0"}},{"name":"system.cpu.wait","value":"0.8","timestamp":1507655859,"tags":{"job":"doppler","index":"","id":"eca69415-6770-467f-a0aa-6cb620a0b6d0"}},{"name":"system.mem.percent","value":"37","timestamp":1507655859,"tags":{"job":"doppler","index":"","id":"eca69415-6770-467f-a0aa-6cb620a0b6d0"}},{"name":"system.mem.kb","value":"1502632","timestamp":1507655859,"tags":{"job":"doppler","index":"","id":"eca69415-6770-467f-a0aa-6cb620a0b6d0"}},{"name":"system.swap.percent","value":"0","timestamp":1507655859,"tags":{"job":"doppler","index":"","id":"eca69415-6770-467f-a0aa-6cb620a0b6d0"}},{"name":"system.swap.kb","value":"4944","timestamp":1507655859,"tags":{"job":"doppler","index":"","id":"eca69415-6770-467f-a0aa-6cb620a0b6d0"}},{"name":"system.disk.system.percent","value":"27","timestamp":1507655859,"tags":{"job":"doppler","index":"","id":"eca69415-6770-467f-a0aa-6cb620a0b6d0"}},{"name":"system.disk.system.inode_percent","value":"14","timestamp":1507655859,"tags":{"job":"doppler","index":"","id":"eca69415-6770-467f-a0aa-6cb620a0b6d0"}},{"name":"system.disk.ephemeral.percent","value":"3","timestamp":1507655859,"tags":{"job":"doppler","index":"","id":"eca69415-6770-467f-a0aa-6cb620a0b6d0"}},{"name":"system.disk.ephemeral.inode_percent","value":"2","timestamp":1507655859,"tags":{"job":"doppler","index":"","id":"eca69415-6770-467f-a0aa-6cb620a0b6d0"}},{"name":"system.healthy","value":"1","timestamp":1507655859,"tags":{"job":"doppler","index":"","id":"eca69415-6770-467f-a0aa-6cb620a0b6d0"}}]}

Notice that the above contains "index":"" not an actual value.
/cc @jtarchie

We are closing this issue because it is fixed in later versions of bosh.
Tested with Bosh 263.


Updated validations.py python script.

Updated validations.py python script.
Fixed the behaviour of validate_user function in validation.py.
Fixes #1

It's a good idea to open an issue first for discussion.

[ ] Tests pass
[ ] Appropriate changes to README are included in PR

@googlebot I signed it!

@googlebot I signed it!

@googlebot I signed it!

@googlebot I fixed it

@googlebot I fixed it

@googlebot I fixed it.

@googlebot I signed it!

@googlebot I fixed it.


[OCPCLOUD-1384] Delete the placement group after machine removal

When we remove a machine, we should also make sure that its placement group is also deleted, but only if it doesn't contain other machines.

/retest


HtTrendline Gives  index out of range

panic: runtime error: index out of range
goroutine 7 [running]:
github.com/markcheno/go-talib.HtTrendline(0xc42025ea00, 0x1a, 0x20, 0x6, 0xc8, 0xc420268274)
/home/codevarun/go/src/github.com/markcheno/go-talib/talib.go:185 +0x10b5

I can't tell if there is a problem with the info given. My guess would be bad input data.

Hi, Same data worked on a wrapper. (github.com/d4l3k/talib)

If you can give me example data to reproduce the problem, I can fix it.


Added Podcast Guru

Thanks so much! :) This is merged (even if it doesn't look like it). Good of you, thanks.

Thanks so much! :) This is merged (even if it doesn't look like it). Good of you, thanks.


Is it possible to add buttons in the preview?

It's a good library to have for previewing image. I was wondering if there is any way to add custom buttons inside the preview like share or delete the image.

@ahsanaasim did you try the pull request that fixes the issue? You may have to update your project a little, as initialising image viewer controllers has slightly changed.


RPM issue

Hello, it seems that the .rpm tool is adding this path on installation "/opt/obuildfactory/jdk-1.9.0-openjdk-x86_64/" and maybe it should have been something like this "/usr/lib/jvm/openjdk-9" - "/usr/lib64/jvm/openjdk-9", since you can't modify the path of the .rpm installation. I tried adding --prefix before installation, but it only adds an additionl path before "/opt/...". Another possibility would have been to only have the path without "/opt/obuildfactory/" and then when using rpm -i -prefix=/usr/lib/jvm it would install in the right place. Also, this kind of approach would add more weight on the end user.

/opt/obuildfactory location is by design, to install jvms outside stock jvms locations and avoid problems.
It's easy to update PATH and JAVA_HOME to enroll them

Also, there is a problem with the rpm files, meaning that they can't be installed on systems, due to this: https://github.com/jordansissel/fpm/issues/1386 . I can make a pull request regarding this.

I close this ticket and wait for your PR


composeExtensions[0].scopes isn't working though it is shown as a required property in the documentation

Without setting the scope to just one of the scopes the message action button shows up twice.

Error: An error occurred while parsing the manifest.json file; Property 'scopes' has not been defined and the schema does not allow additional properties. Path 'composeExtensions[0].scopes', line 49, position 21.

@kartikluke - Message extension does not support scopes. That's the reason you are getting error while parsing.

For multiple entries issue:
This is happening because every time you sideload an app, different id is assigned to it. So, you have different app in personal scope and different in team scope.
Try any of these to get rid of multiple entries:

Uninstall your personal scope app. Now, it will show only one entry from the app installed in the team scope.
Uninstall both in team and personal scope. Now install only in Team scope. Click on the app name link and then chat icon. This will take you to your personal app.

Now click on Add button which pops up at the top.

Install your app in Private App store.

Please let us know if you face any other issue.


Is it possible to dismiss the presented controller programatically?

Is there any way for me to dismiss the presented viewcontroller programmatically? Something that does the opposite of customPresentViewController(_:viewController:animated:completion:)?
NVM. dimiss(:animated:completion:) does the job just fine.

Yes!
Thanks for using Presentr 👍


Add a11y Storybook addon

Hi! I watched Pedro’s presentation of Modulz this afternoon and it looks amazing, good job! It prompted me to play around with radix and do some quick accessibility checks to test the primitives 🙈
This PR
Although I’m guessing you’ll move away from Storybook at some point, I’ve noticed a few accessibility issues in the Radix components. This PR sets up a storybook addon that should help avoid those in the future.
The addon uses axe under the hood, which is an accessibility testing engine. It can sometimes report false positives but I didn’t notice any while playing around, and you can totally customize axe to ignore some errors, so IMO there is never a strong reason not to install this addon, but that’s obviously up to you! I set it up during my tests so I figured I could just open a PR.
Here’s an example of what it looks like when browsing the Button component:

It also adds a color blindness control which can be handy:

Will Modulz include something similar, or at least a color contrast check? That’d be a great feature. :)
Accessibility issues
I know Radix and your primitives are WIP but here’s a non exhaustive list of a11y issues I found, in no particular order (some of them are not reported by the addon).
In Radix

The blue variant doesn’t pass the WCAG contrast ratio test (in both Button and Badge). Same for the normal variant of Code, the highlight variant of Link, the fade variant of the ToggleButton and the active variant of List.
The waiting button has a focus outline but it’s almost invisible.
The Switch and Checkbox components don’t have focus states, but you have some opened issues for those already. There’s no issue for the Slider component, though, which also lacks a focus state.
Your icons are not accessible since they do not label the svgs correctly. But that’s probably an issue for the radix-icons repository?

Happy to open separate issues for those.
Directly in the primitives

The inputs/textareas are not using (or enforcing the use of) label elements. You should probably add a visually hidden label to those controls by default. The placeholder attribute is not good enough. If you decide to keep using placeholder, you could start by improving its color contrast so that it passes the WCAG contrast ratio check.
The checkboxes, sliders and switches don’t use label elements either (or don’t link their p label using aria-labelledby), which actually leads to UX issues like the fact that clicking the label of the checkboxes doesn’t toggle the checkbox.
The disabled attribute is used for buttons/inputs but it leads to them being undiscoverable since those are not focusable, which could lead to screen-reader users getting confused. 😞The solution here would be to output aria-disabled="true" instead, style them yourself and make sure all mouse and keyboard events are ignored.

I’m guessing the same primitives are also used in Modulz projects? Do you think accessibility should be handled in user-land, which I guess also gives users more flexibility on the DOM structure? IMO if it could be enforced directly through the primitives it’d be awesome, but you probably have given more thought to this than I did. 😅
I also would have loved to contribute to the primitives, hope you’ll make them open-source at some point!

Hey @Tom-Bonnike Thanks for this! The storybook addon looks cool, might add it to our lower-level primitives repo.
I'll respond to a few of the things you brought up down here:
Radix

The blue variant doesn’t pass the WCAG contrast ratio test (in both Button and Badge). Same for the normal variant of Code, the highlight variant of Link, the fade variant of the ToggleButton and the active variant of List.

The waiting button has a focus outline but it’s almost invisible.

Regarding color contrast accessibility, I'll bring in @vladmoroz our designer to the discussion.

The Switch and Checkbox components don’t have focus states, but you have some opened issues for those already. There’s no issue for the Slider component, though, which also lacks a focus state.

Yep, 100% right, we have open issues to look at these. I'll add one for the Slider now, but it might be because this one is due to be re-implemented in a custom way. At the moment it uses <input type="range /> which makes it impractical to style some states/parts.

Your icons are not accessible since they do not label the svgs correctly. But that’s probably an issue for the radix-icons repository?

Yes by default these are just SVG's really. I believe the accessibility for these should come at a higher level, once we implement something like an IconButton that's where it'll be baked in most likely with an aria-label and and aria-hidden on the SVG. Was that what you had in mind?
Primitives

The inputs/textareas are not using (or enforcing the use of) label elements. You should probably add a visually hidden label to those controls by default. The placeholder attribute is not good enough. If you decide to keep using placeholder, you could start by improving its color contrast so that it passes the WCAG contrast ratio check.

The checkboxes, sliders and switches don’t use label elements either (or don’t link their p label using aria-labelledby), which actually leads to UX issues like the fact that clicking the label of the checkboxes doesn’t toggle the checkbox.

100% agree for using real labels rather than placeholder! 💯
The reason nothing in baked into those is that we currently see these as the real low-level leaf components that you can style. So they are literally just the control (input, checkbox, radio, etc).
I think the accessibility would come rather from a higher-level component like a Field where you can compose a label and a control, etc.

The disabled attribute is used for buttons/inputs but it leads to them being undiscoverable since those are not focusable, which could lead to screen-reader users getting confused. 😞The solution here would be to output aria-disabled="true" instead, style them yourself and make sure all mouse and keyboard events are ignored.

That's a great shout. We are actually doing some of this in places already like Tabs so screen readers are aware of disabled tabs. We'll give some thoughts and considerations to extending this to other controls. In your experience, are there any downsides in doing this for most controls like button, input, checkbox, etc?
Thanks again for you time @Tom-Bonnike!

The reason nothing in baked into those is that we currently see these as the real low-level leaf components that you can style. So they are literally just the control (input, checkbox, radio, etc).
I think the accessibility would come rather from a higher-level component like a Field where you can compose a label and a control, etc.

I see, makes total sense, but do you think you’ll have higher-level primitives for those in Modulz, or do you see accessibility as your user’s responsibility?
That’s fair, but I’d personally love to see a design tool that actually enforces accessibility or at least forces people to think about it directly at the design stage, at least for “simpler” stuff like color contrast, forms and images. It would also also force designers to think about all states (e.g. errored fields, which also require a bit of work to be accessible). So yes, this PR is me lobbying for Modulz to make my job as a developer easier 👹Not sure how that would all look like in practice without limiting the users too much, though.

Yes by default these are just SVG's really. I believe the accessibility for these should come at a higher level, once we implement something like an IconButton that's where it'll be baked in most likely with an aria-label and and aria-hidden on the SVG. Was that what you had in mind?

Here’s the simplified Icon component we used at my previous job, we had an accessibility audit done recently which confirmed we were doing things correctly:
// Assuming TextAlignLeftIcon is a component rendering a <svg> tag, we used it like this:
// <Icon icon={TextAlignLeftIcon} />

const Icon = props => {
  if (!props.icon) return

  if (!props.label) {
    return (
      <props.icon
        aria-hidden
        focusable='false'
      />
    )
  }

  // The `role` and `aria-live` attributes might be useful in case the icon
  // needs to provide further information to the accessibility tree such as
  // for a loading spinner.
  return (

      <props.icon
        aria-hidden
        focusable='false'
      />
      <VisuallyHidden>{props.label}</VisuallyHidden>

  )
}

Although you don’t need to label decorative icons when they’re accompanying text, apparently adding aria-hidden and focusable="false" on svgs is recommended to have them properly ignored by all screen-readers.
But, for example, the first example with the icon in your ToggleButton story is not accessible, obviously it’s “just” a Storybook story and not a real use-case, but if the icons had a required label prop, it would be impossible to forget about it, and it still could be manually set to an empty string when you’re sure you don’t want one.

We are actually doing some of this in places already like Tabs so screen readers are aware of disabled tabs.

Great 👍

In your experience, are there any downsides in doing this for most controls like button, input, checkbox, etc?

I was surprised to discover this after an accessibility audit at my last job, but because using disabled on buttons and inputs make them unfocusable, it can lead screen-reader users to dead end because
they won’t easily be able to discover them and so won’t be able to submit the form if there isn’t any hint.
Disabled inputs are imo anyway an antipattern, unless having an unactionable yet visible dimmed field is what you actually want, but in most cases you’re probably looking for a readonly input instead 😄
Form buttons should always be actionable and make users jump back to the field that’s invalid, or there should be something that explains clearly why the button is disabled.
We had “fake” disabled buttons with aria-disabled="true" that were styled like a disabled one and ignored clicks (pointer-events: none) and keypresses on them for places where we really needed a disabled button.

❤ this @Tom-Bonnike, keep it coming!

I see, makes total sense, but do you think you’ll have higher-level primitives for those in Modulz, or do you see accessibility as your user’s responsibility?
That would be fair, but I’d personally love to see a design tool that actually enforces accessibility or at least forces people to think about it directly at the design stage, at least for “simpler” stuff like color contrast, forms and images. It would also also force designers to think about all states (e.g. errored fields, which also require a bit of work to be accessible). So yes, this PR is me lobbying for Modulz to make my job as a developer easier 👹Not sure how that would all look like in practice without limiting the users too much, though.

I think some accessibility will always lie within the responsibility of the app developers because there are many things that can't be automated. That being said, it is most definitely our goal to automate and give for free as much as possible. So we would definitely be looking at these kinds of higher level components which wouldn't necessarily feature in Modulz itself but would be available in the code export.
An existing example of this already is our DropdownMenu component. It doesn't appear in Modulz, it's simplify a functional aid. What you see and get to style in Modulz is a Menu component (item, separator, etc). This Menu component gets used in a few places, like DropdownMenu, RightClickMenu (another example of functional aid) and under the hood of the Select component. The DropdownMenu then takes any button (could be one from your DS or not) and a Menu component and wires it all together for you with the proper accessibility (menu-button aria pattern). Feel free to take a look at it in the radix styleguide (DropdownMenu component.).

Yup! Here’s the simplified Icon component we used at my previous job, we had an accessibility audit done recently which confirmed we were doing things correctly:

I like this a lot. We currently don't have this because we only have radix icons available for us. The slightly longer term plan is to enable different types of icon libraries through Modulz and enabling users to pick an icon in places (think Tick for a checkbox, etc). I don't see why we can't start thinking about such a component though, love it. Love the extra attention for role, etc. I'm gonna put an issue for us to look at this, seems like a fairly easy win.

Although you don’t need to label decorative icons when they’re accompanying text, apparently adding aria-hidden and focusable="false" on svgs is recommended to have them properly ignored by all screen-readers.

Yep, aria-hidden makes sense.
Can I ask about the focusable='false'? Is this a regular DOM attribute? A special prop?

But, for example, the first example with the icon in your ToggleButton story is not accessible, obviously it’s “just” a Storybook story and not a real use-case, but if the icons had a required label prop, it would be impossible to forget about it, and it still could be manually set to an empty string when you’re sure you don’t want one.

Yep for sure stories won't always be nice and up to date.
Also some components don't appear in the Radix styleguide but we do have a VisuallyHidden component in the export and in that story we have:
<>

Screen readers will enunciate the button below as "Notifications, button."

  <button type="button">
    <VisuallyHidden>Notifications</VisuallyHidden>
    <BoxIcon aria-hidden />
  </button>
</>

We had “fake” disabled buttons with aria-disabled="true" that were styled like a disabled one and ignored clicks (pointer-events: none) and keypresses on them for places where we really needed a disabled button.

Yeah that's interesting, that's partly what we do in Tabs for example. And interestingly, we've also recently updated most of these controls primitive to use pointer-events: none when disabled for styling reason (because it simplifies things like making sure hover styles can't work on disabled controls, I've never understood why the browser lets that happen.)
👍

That being said, it is most definitely our goal to automate and give for free as much as possible.

Good to hear 👍

Can I ask about the focusable='false'? Is this a regular DOM attribute? A special prop?

I looked up online because I couldn’t remember and it’s an attribute that’s only valid on <svg>s. Apparently some browsers consider them as focusable elements by default (IE11 and Edge I think?) and this helps make sure they can never receive focus.
Cool, sounds good, I hope to get the chance to use Modulz in a team soon. :)

The blue variant doesn’t pass the WCAG contrast ratio test (in both Button and Badge). Same for the normal variant of Code, the highlight variant of Link, the fade variant of the ToggleButton and the active variant of List.

Thanks for the catch @Tom-Bonnike, I think we only tested this blue color on white only, and not on the off-white that these components use. We are taking a look at colors soon, will make sure to update it.

The waiting button has a focus outline but it’s almost invisible.

Didn't even know! That slipped through. We are plan to review these buttons at some point and replace the waiting variant with a more suitable component.
Let me know if you saw anything else, thank you 🙏