{"action":"created","issue":{"url":"https://api.github.com/repos/mozilla/pdf.js/issues/6273","labels_url":"https://api.github.com/repos/mozilla/pdf.js/issues/6273/labels{/name}","comments_url":"https://api.github.com/repos/mozilla/pdf.js/issues/6273/comments","events_url":"https://api.github.com/repos/mozilla/pdf.js/issues/6273/events","html_url":"https://github.com/mozilla/pdf.js/pull/6273","id":97324981,"number":6273,"title":"Refactor annotation code to use a factory","user":{"login":"timvandermeij","id":1993262,"avatar_url":"https://avatars.githubusercontent.com/u/1993262?v=3","gravatar_id":"","url":"https://api.github.com/users/timvandermeij","html_url":"https://github.com/timvandermeij","followers_url":"https://api.github.com/users/timvandermeij/followers","following_url":"https://api.github.com/users/timvandermeij/following{/other_user}","gists_url":"https://api.github.com/users/timvandermeij/gists{/gist_id}","starred_url":"https://api.github.com/users/timvandermeij/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/timvandermeij/subscriptions","organizations_url":"https://api.github.com/users/timvandermeij/orgs","repos_url":"https://api.github.com/users/timvandermeij/repos","events_url":"https://api.github.com/users/timvandermeij/events{/privacy}","received_events_url":"https://api.github.com/users/timvandermeij/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/mozilla/pdf.js/labels/4-annotations","name":"4-annotations","color":"DDDDDD"}],"state":"open","locked":false,"assignee":{"login":"Snuffleupagus","id":2692120,"avatar_url":"https://avatars.githubusercontent.com/u/2692120?v=3","gravatar_id":"","url":"https://api.github.com/users/Snuffleupagus","html_url":"https://github.com/Snuffleupagus","followers_url":"https://api.github.com/users/Snuffleupagus/followers","following_url":"https://api.github.com/users/Snuffleupagus/following{/other_user}","gists_url":"https://api.github.com/users/Snuffleupagus/gists{/gist_id}","starred_url":"https://api.github.com/users/Snuffleupagus/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Snuffleupagus/subscriptions","organizations_url":"https://api.github.com/users/Snuffleupagus/orgs","repos_url":"https://api.github.com/users/Snuffleupagus/repos","events_url":"https://api.github.com/users/Snuffleupagus/events{/privacy}","received_events_url":"https://api.github.com/users/Snuffleupagus/received_events","type":"User","site_admin":false},"milestone":null,"comments":12,"created_at":"2015-07-26T15:01:25Z","updated_at":"2015-07-29T07:22:12Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/mozilla/pdf.js/pulls/6273","html_url":"https://github.com/mozilla/pdf.js/pull/6273","diff_url":"https://github.com/mozilla/pdf.js/pull/6273.diff","patch_url":"https://github.com/mozilla/pdf.js/pull/6273.patch"},"body":"Currently, `src/core/core.js` uses the `fromRef` method on an `Annotation` object to obtain the right annotation type object (such as `LinkAnnotation` or `TextAnnotation`). That method in turn uses a method `getConstructor` to find out which annotation type object must be returned.\r\n\r\nAside from the fact that there is currently a lot of code to achieve this, these methods should not be part of the base `Annotation` class at all. Creation of annotation object should be done by a factory (as also recommended by @yurydelendik at https://github.com/mozilla/pdf.js/pull/5218#issuecomment-52779659) that handles finding out the correct annotation type object and returning it. This patch implements this separation of concerns.\r\n\r\nDoing this allows us to also simplify the code quite a bit and to make it more readable. Additionally, we are now able to get rid of the hardcoded array of supported annotation types. The factory takes care of checking the annotation types and falls back to returning the base annotation type (and issuing a warning, which the current code also does not do well) when an annotation type is unsupported.\r\n\r\nI have manually tested this commit with 20 test PDFs with different annotation types, such as /Link, /Text, /Widget, /FileAttachment and /FreeText. All render identically before and after the patch, and unsupported annotation types are now properly indicated with a warning in the console.\r\n\r\n@Snuffleupagus Would you be so kind again to review this? To test this, you can use all PDFs in issues with the `4-annotation` label (both open and closed) and check rendering and the console before and after the patch. If you want even more test files, I can provide the 20 test PDFs I used to test this, but I think the `4-annotation` label contains enough PDFs to properly verify this. A sample PDF to see that warnings are now properly put in the console is http://mirrors.rit.edu/CTAN/macros/latex/contrib/pdfcomment/doc/example.pdf."},"comment":{"url":"https://api.github.com/repos/mozilla/pdf.js/issues/comments/125868139","html_url":"https://github.com/mozilla/pdf.js/pull/6273#issuecomment-125868139","issue_url":"https://api.github.com/repos/mozilla/pdf.js/issues/6273","id":125868139,"user":{"login":"pdfjsbot","id":1036361,"avatar_url":"https://avatars.githubusercontent.com/u/1036361?v=3","gravatar_id":"","url":"https://api.github.com/users/pdfjsbot","html_url":"https://github.com/pdfjsbot","followers_url":"https://api.github.com/users/pdfjsbot/followers","following_url":"https://api.github.com/users/pdfjsbot/following{/other_user}","gists_url":"https://api.github.com/users/pdfjsbot/gists{/gist_id}","starred_url":"https://api.github.com/users/pdfjsbot/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/pdfjsbot/subscriptions","organizations_url":"https://api.github.com/users/pdfjsbot/orgs","repos_url":"https://api.github.com/users/pdfjsbot/repos","events_url":"https://api.github.com/users/pdfjsbot/events{/privacy}","received_events_url":"https://api.github.com/users/pdfjsbot/received_events","type":"User","site_admin":false},"created_at":"2015-07-29T07:22:12Z","updated_at":"2015-07-29T07:22:12Z","body":"#### From: Bot.io (Windows)\n\n----\n\n# Failed\n\nFull output at http://107.22.172.223:8877/75c2907d924e2d7/output.txt\n\nTotal script time: 2.91 mins\n\n+ **Font tests:** FAILED\n+ **Unit tests:** Passed\n+ **Regression tests:** FAILED\n\nImage differences available at: http://107.22.172.223:8877/75c2907d924e2d7/reftest-analyzer.html#web=eq.log"}}