{"action":"created","issue":{"url":"https://api.github.com/repos/mozilla/pdf.js/issues/6770","labels_url":"https://api.github.com/repos/mozilla/pdf.js/issues/6770/labels{/name}","comments_url":"https://api.github.com/repos/mozilla/pdf.js/issues/6770/comments","events_url":"https://api.github.com/repos/mozilla/pdf.js/issues/6770/events","html_url":"https://github.com/mozilla/pdf.js/pull/6770","id":122754276,"number":6770,"title":"Refactor annotation display layer code to use classes","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":10,"created_at":"2015-12-17T15:10:47Z","updated_at":"2015-12-19T00:29:52Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/mozilla/pdf.js/pulls/6770","html_url":"https://github.com/mozilla/pdf.js/pull/6770","diff_url":"https://github.com/mozilla/pdf.js/pull/6770.diff","patch_url":"https://github.com/mozilla/pdf.js/pull/6770.patch"},"body":"This patch refactors the annotation display layer code to use classes. This work makes it easier to add new annotation types and improves readability and testability of the code. Feedback is appreciated.\r\n\r\nMost of this patch is moving existing code. The following structural additions have been made:\r\n\r\n- Introduction of `AnnotationElementFactory`, which is based on the code in `src/core/annotation.js`, to handle returning the correct annotation element type object.\r\n- Introduction of `AnnotationElement`, a base class for other annotation element types that is responsible for creating a generic empty container.\r\n- Introduction of `LinkAnnotationElement`, responsible for rendering a link annotation element. Contains existing code, but `_bindLink` and `_bindNamedAction` have been moved to separate methods where they were previously inlined.\r\n- Introduction of `TextAnnotationElement`, responsible for rendering a text annotation element. Contains existing code, but `_toggle`, `_show` and `_hide` have been moved to separate methods where they were previously inlined.\r\n- Introduction of `WidgetAnnotationElement`, responsible for rendering a widget annotation element. Contains existing code with `_setTextStyle` as a method here instead of an overall function before.\r\n\r\nAside from the above, JSDoc comments have been added to the classes and methods.\r\n\r\nI have tested this with a set of 23 PDF files containing different annotation types, where Link, Text and Widget are covered. I found no difference in behavior, rendering or printing before and after the patch.\r\n\r\n@Snuffleupagus Do you have time to review this? If not, feel free to unassign yourself. This patch follows the same pattern as `src/core/annotation.js` and it's primarily moving around existing code, so hopefully it's not too hard to review. Using `?w=1` might help a bit. Let me know if you would like me to split this into more commits. I can, for example, extract the JSDoc comment additions into a separate commit. Extracting more will be hard because then each commit would not be a working version on its own."},"comment":{"url":"https://api.github.com/repos/mozilla/pdf.js/issues/comments/165926738","html_url":"https://github.com/mozilla/pdf.js/pull/6770#issuecomment-165926738","issue_url":"https://api.github.com/repos/mozilla/pdf.js/issues/6770","id":165926738,"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},"created_at":"2015-12-19T00:29:52Z","updated_at":"2015-12-19T00:29:52Z","body":"Thank you both. I will address the comments soon. @Snuffleupagus, there is now #6780 for the regression testing."}}