| | <<<Back 1 day (to 2018/08/20) | 20180821 |
| inflex | tor8, managed to sort out everything well enough that it's functioning as I want including with left/right cursor positioning. | 07:56.25 |
| paulgardiner | Another tiny commit on my master up for review. | 09:28.05 |
| | tor8: I'm planning on resynching muso's mupdf submodule soon. There's a number of bugs fixed. It would be handy to get your fixes for 699625 and 699641 in if they are ready to go. They seemed to work well when I tested them. | 09:29.29 |
| tor8 | paulgardiner: you mention the wrong commit sha1 in your message, the commit that removes the val = pdf_to_name line is f533104d6e66b3fc6d3b63b98ec7fe4fb175b366 | 09:42.06 |
| paulgardiner | Bah! I must not be understanding how git gui's blame thing works. | 09:42.53 |
| tor8 | you went back one commit too far for that line change | 09:43.09 |
| | I think it's just a mistake in that commit | 09:43.29 |
| paulgardiner | Ah right. | 09:43.39 |
| | I could see the sense of it. There were two lines that had been changed no longer to need val, and the later use was a fair but further through the function. | 09:44.35 |
| tor8 | though looking at that function, I think it's not entirely correct | 09:44.56 |
| | the /V of a checkbox widget should be a name object, not a string | 09:45.13 |
| paulgardiner | Oh okay. I'll take a look | 09:45.31 |
| tor8 | sebras prompted me a few days ago and I read through the bit of the spec and found that indeed, widget values are not always text strings :( | 09:46.14 |
| | the /V entry for radio buttons and check boxes should be name objects | 09:46.55 |
| paulgardiner | So should I just change pdf_new_text_string to pdf_new_name at the end of toggle_check_box?> | 09:47.11 |
| tor8 | page 686 of pdfref17 mentions it | 09:47.29 |
| | I think we can remove the whole to-string conversion round | 09:47.53 |
| paulgardiner | Just use key? | 09:48.38 |
| tor8 | let me poke around a bit | 09:48.54 |
| paulgardiner | Sure. Thanks. | 09:49.04 |
| | I can send you the file that showed the bug that provoked me to look at this, if you like. | 09:49.46 |
| tor8 | that would be handy | 09:50.49 |
| paulgardiner | Sent | 09:53.26 |
| tor8 | paulgardiner: hm, toggle_check_box only seems to be touching the /AS of the other buttons in the group, never the /V field | 09:59.42 |
| | and the /V field of the clicked button is only changed if 'grp' is true | 09:59.57 |
| | not that I understand exactly what 'grp' is supposed to mean | 10:00.35 |
| | see tor/master for what I mean | 10:03.20 |
| paulgardiner | Nor me, I'm afraid. It's been a long time. It make sense in the radio case, but I forget what it's for in the non-radio case. I remember there are cases where buttons are ganged, so setting one will set them all, but I don't see how this relates to that. | 10:04.00 |
| tor8 | if we only update the /AS scripts that tweak on the checkbox values may end up wrong? | 10:04.34 |
| paulgardiner | I guess I didn't write find_head_of_field_group on a whim, so there must have been a need. | 10:04.48 |
| tor8 | I'm wondering if there's a case where we can have a radio/checkbox that doesn't have 'grp' set | 10:05.33 |
| | I think no, because all forms should have a /Parent shouldn't they, and that points back to the /AcroForm at the top? | 10:05.47 |
| paulgardiner | Also there's if (!grp) grp = obj | 10:06.08 |
| tor8 | oh, no, it's optional | 10:06.10 |
| | ah, missed that line! | 10:06.15 |
| | so, then the if (grp) line that guards the set_check_grp will never hit the 'else' case | 10:06.56 |
| paulgardiner | Oh yeah. Hmmm! | 10:07.42 |
| | That alternation makes perfect sense as code that was present before the addition of if (!grp) grp = obj | 10:08.53 |
| tor8 | I think the JS code can't handle non-text /V values somewhere there | 10:09.45 |
| | because if I put a name object as the value, nothing appears in the KOV box | 10:10.03 |
| paulgardiner | I cannot remember the interplay between V an AS for buttons. | 10:10.31 |
| | I'll go read the spec again, see if I can figure out exactly what grp is doing. | 10:11.36 |
| tor8 | field_getValue assumes the /V field is a text string or stream | 10:12.52 |
| | and can't cope with it being a name | 10:12.58 |
| paulgardiner | I guess that's easily fixed... but why again did you think setting to a string is wrong? | 10:14.21 |
| | V is inheritable, so setting it only on the grp may make sense. | 10:16.25 |
| tor8 | "The V entry in the field dictionary (see Table 8.69 on page 675) holds a name ob- | 10:16.56 |
| | ject representing the check boxâs appearance state, which is used to select the ap- | 10:16.57 |
| | propriate appearance from the appearance dictionary. " | 10:16.57 |
| | and "The parent fieldâs V entry holds a name object corresponding to the ap- | 10:18.04 |
| | pearance state of whichever child field is currently in the on state; the default val- | 10:18.04 |
| | ue for this entry is Off." | 10:18.04 |
| | the parent's field is I think the 'grp' | 10:18.17 |
| | so I think I understand the 'grp' and why it's setting the grp's V to the currently selected radio button's AS | 10:19.19 |
| paulgardiner | For radio buttons yes. I'm not sure why I look further up the hierarchy for the non-radio case | 10:19.20 |
| tor8 | and find_head_of_field_group looks for the first field with a name | 10:20.14 |
| | in the parent chain | 10:20.19 |
| | so I guess both of those make sense | 10:20.25 |
| paulgardiner | Can you give me a link to the version of the PDF spec you use. I have the ISO one and the pages never line up | 10:20.32 |
| | page numbers | 10:20.40 |
| tor8 | I use the pdfref17.pdf | 10:20.44 |
| | the last one with good typography | 10:20.47 |
| | you can find it on casper as ~tor/pdfref17.pdf | 10:22.34 |
| paulgardiner | Ah yes. Looking for the first with a name makes sense. | 10:22.43 |
| kens | And more importantly, an index.... | 10:22.54 |
| | The ISO version 2 spec lacks an index, and opens with the outlines all opened up, which makes navigation impossible | 10:23.25 |
| | And the lack of an index makes it impossible to find the documentation on a particular feature | 10:23.55 |
| | Its a typicall ISO spec, basically. More or less unusable | 10:24.12 |
| paulgardiner | So maybe, removing the unnecessary alternation, setting V to key and updating field_getValue to accept names would do it. | 10:25.43 |
| tor8 | paulgardiner: take a peek at tor/master (I just updated it again) | 10:28.09 |
| paulgardiner | Looks great. You could keep the "break", but it I guess it would make next to no difference. | 10:33.39 |
| tor8 | thanks. I'll clean that up and prepare my branch for review then. | 10:35.14 |
| sebras | reads logs. | 10:35.40 |
| paulgardiner | Thanks Tor | 10:36.03 |
| sebras | tor8: yeah, sorry about that. ;) | 10:45.37 |
| kens | sebras ping | 10:47.23 |
| sebras | kens: how did I break the windows build this time? | 10:49.04 |
| | ;) | 10:49.13 |
| kens | I've no idea, did you do that ? | 10:49.13 |
| | I actually wanted to ask you about : | 10:49.29 |
| | https://bugs.ghostscript.com/show_bug.cgi?id=699271 | 10:49.29 |
| | Did you see comment #4 ? | 10:49.38 |
| sebras | kens: no I just imagined that's what you were going to ask m3e about. :) | 10:49.39 |
| | kens: let me take a look. | 10:49.49 |
| kens | Ah well, I'd probably have pinged you on another channel | 10:49.53 |
| | "Carlo B" uses a more or less anonymous email address, but quotes his company website.... | 10:50.22 |
| paulgardiner | tor8: one more on my master that maybe we can roll in. | 11:10.22 |
| tor8 | paulgardiner: sure, that one LGTM. I'll roll it in. | 11:14.31 |
| paulgardiner | ta | 11:16.09 |
| tor8 | paulgardiner: sebras: could you review all but the two WIP commits on tor/master? | 11:20.19 |
| sebras | sure. | 11:21.09 |
| paulgardiner | looking | 11:23.48 |
| | They all look fine to me. | 11:25.29 |
| sebras | tor8: pdfref17.pdf page 673 table 8.67 entry DA | 11:33.46 |
| | would this be the parent at the top? | 11:34.36 |
| | or would this require a separate lookup after walking up the /Parent hierarchy? | 11:35.29 |
| tor8 | a separate lookup (as is done in pdf_annot_default_appearance) | 11:38.49 |
| sebras | it happened below the context of the patch, sorry. :) | 11:40.24 |
| | I'm confused about "Fix 699625: Apply more vertical padding for multiline form fields." | 11:41.24 |
| | is the order of testing multiline vs comb important? why? | 11:41.35 |
| tor8 | sebras: they are mutually exclusive, so shouldn't matter | 11:41.51 |
| | but I changed the order to match the behavior of poppler | 11:42.02 |
| sebras | also how can we get _more_ padding when the second argument to Td represent Ty and gets smaller? | 11:42.31 |
| | I'm looking at write_variable_text(). | 11:42.52 |
| | or size - baseline was negative before? | 11:43.14 |
| tor8 | sebras: that particular bit of padding is smaller | 11:43.42 |
| | it's the call to write_variable_text that adds more constant padding | 11:44.00 |
| sebras | really? I guess that depends on the value of b, b * 2 vs b + 3 | 11:44.33 |
| tor8 | these numbers are just randomly guessed to roughly correspond to what adobe looks to be doing | 11:44.41 |
| | but determining exactly what adobe does is beyond me at the moment | 11:44.51 |
| | I've looked and prodded and can't find a coefficient or factor that makes any sense to me | 11:45.09 |
| sebras | what confuses me is the word "more" in "Apply more vertical padding". perhaps we are just doing it similar to acroread? | 11:45.33 |
| tor8 | it is much more padding than we had before (the +3 is larger than the minor size/baseline difference adjustment I tried using before) | 11:46.46 |
| sebras | ok. | 11:46.59 |
| | tor8: everything up to, but not including "Fix form recalculation issue." LGTM. | 11:51.40 |
| | I'm still reviewing that last one. | 11:51.46 |
| | who wrote this part of the spec? ffs... : | 12:13.17 |
| | :( | 12:13.20 |
| | why does find_head_of_field_group() check for /T rather than if a particular object is present in a fields /Kids array? | 12:23.06 |
| | would couldn't a button itself have a /T entry? it doesn't seem like the spec if prohibiting that anyway. | 12:23.59 |
| | anywhere. | 12:24.04 |
| tor8 | sebras: paulgardiner might be able to explain that one better | 12:29.30 |
| | but the /T entry is what is used by the javascript bindings to query/set field values | 12:29.44 |
| sebras | tor8: if you change pdf_dict_getp() to pdf_dict_getl() in toggle_check_box() would it make sense to do the same in set_check()? | 12:30.41 |
| | and possibly recalculate(). | 12:30.49 |
| paulgardiner | I don't understand the question, as in I don't get why checking for a field in Kids might make better sense. | 12:32.09 |
| sebras | paulgardiner: isn't that how the hierarcy is expressed? at least in the case of radio/check boxes? | 12:33.53 |
| paulgardiner | If you follow Parent links, you know the field you start from is in Kids or Kids/Kids (unless the file is broken). I think I'm confused. | 12:36.48 |
| | A whole load of fields are given a single name by setting T above them in the hierarchy, but not setting it anywhere lower. | 12:37.47 |
| sebras | paulgardiner: ok, but why does /T terminate the recursive lookup through /Parent fields? | 12:38.33 |
| | paulgardiner: i.e. why do we look for /T in find_head_of_field_group() as a reason to terminate the loop? | 12:38.58 |
| paulgardiner | Hmmm. Maybe we shouldn't stop at the first T, but continue to just before T changes again | 12:39.07 |
| sebras | what if a radio button field group does not have a /T entry but has a /Kids array? | 12:39.23 |
| paulgardiner | For radio buttons, we just follow one Parent link | 12:40.26 |
| sebras | but that is not the case for check boxes. | 12:41.35 |
| | also my brain is struggling to understand what happens then the widget annotation and its contents are merged into the field dirctionary itself and the Kids array is omitted as per the /Kids explanation on pdfref17.pdf 675. | 12:42.45 |
| | I guess that means you may have a radio field with just a single radio button in which case the field dictionary is also the same as the widget. | 12:43.30 |
| paulgardiner | If there is no name, there is no group. What concerns me is whether there could be a name used multiple times but done other than by labelling just one point in the hierarchy. | 12:43.59 |
| sebras | and if Ff has no NoToggleToOff then it looks like a radio button but behaves like a check box. | 12:44.19 |
| paulgardiner | I don't believe merging is done when there is any sort of hierarchy, although perhaps the spec doesn't preclude that. | 12:46.41 |
| | I'm struggling a lot to remember any of this. It was a long time ago. | 12:47.07 |
| sebras | paulgardiner: no name => no group, is that true? why does the example in 8.17 have to have a /T entry? | 12:47.44 |
| | paulgardiner: mmm, I believe that the spec allows more than what makes sense. | 12:48.11 |
| paulgardiner | I don't know. I thought groups were defined by the name | 12:48.15 |
| sebras | paulgardiner: I'm not so sure about that. | 12:49.29 |
| paulgardiner | What possibly we should be doing is scanning the whole form and not trusting that any one name appears just once in the hierarchy, within a node that has all the fields of that name as descendents | 12:49.45 |
| sebras | paulgardiner: but I'll can be swayed by a good argument! :) | 12:49.46 |
| paulgardiner | I have no good arguments. I think quite possibly this algorithm isn't quite right, but still Tor's change is at least an improvement, and at least fixes the regression. | 12:51.04 |
| sebras | paulgardiner: the spec does say on p677 that different field dicts may have the same fully qualified field dict name if they are descendants of a common ancestor and have no names of their own. is there something preventing fields from having the name /T entry and thus having the same name though? | 12:52.17 |
| paulgardiner | I have to admit, I don't know. It may well be that I went with the current simple method just because all the example files I studied followed that pattern. | 12:54.09 |
| sebras | ok | 12:54.39 |
| | tor8: now that I understand more I can see that "Fix form recalculation issue." is an improvement, but this area probably needs further comparisons with the spec. | 12:58.34 |
| tor8 | sebras: yes. | 12:58.47 |
| paulgardiner | The current handling of groups is sort of reading p677 as iff rather than if, and - who knows - that might have been the intention. | 13:03.52 |
| sebras | do you mind taking at look at 699271, 699652 and 699653 on sebras/master? the latter two now have bug numbers and has been updated as we discussed. | 14:05.54 |
| | they are in the cluster lotto. | 14:07.41 |
| moriarty | howdy folks | 16:52.31 |
| | Forward 1 day (to 2018/08/22)>>> | |
Log of #mupdf at irc.freenode.net.