GitHub PullRequestReviewCommentEvent at 2014-12-16 12:04:02
Repository: shoes4
Actor: PragTob
Payload: {"action":"created","comment":{"url":"https://api.github.com/repos/shoes/shoes4/pulls/comments/21892972","id":21892972,"diff_hunk":"@@ -0,0 +1,156 @@\n+require 'shoes/swt/spec_helper'\n+\n+describe Shoes::Swt::ClickListener do\n+  include_context 'swt app'\n+\n+  let(:dsl) { double('dsl', pass_coordinates?: false) }\n+  let(:swt) { double('swt', dsl: dsl) }\n+\n+  let(:click_block)   { double(\"click block\", call: nil) }\n+  let(:release_block) { double(\"release block\", call: nil) }\n+\n+  subject   { Shoes::Swt::ClickListener.new(swt_app) }\n+\n+  before do\n+    allow(swt_app).to receive(:add_listener)\n+  end\n+\n+  describe \"SWT event wireup\" do\n+    it \"registers mouse down and mouse up\" do\n+      subject\n+      expect(swt_app).to have_received(:add_listener).with(::Swt::SWT::MouseDown, subject)\n+      expect(swt_app).to have_received(:add_listener).with(::Swt::SWT::MouseUp, subject)\n+    end\n+  end\n+\n+  describe \"adding listeners\" do\n+    it \"adds a click listener\" do\n+      subject.add_click_listener(swt, click_block)\n+      expect(subject.clickable_elements).to eq([dsl])\n+    end\n+\n+    it \"adds a release listener\" do\n+      subject.add_release_listener(swt, release_block)\n+      expect(subject.clickable_elements).to eq([dsl])\n+    end\n+\n+    it \"only reports each element once\" do\n+      subject.add_click_listener(swt, click_block)\n+      subject.add_release_listener(swt, release_block)\n+      expect(subject.clickable_elements).to eq([dsl])\n+    end\n+  end\n+\n+  describe \"removing\" do\n+    it \"removes safely when not added\" do\n+      subject.remove_listeners_for(dsl)\n+      expect(subject.clickable_elements).to be_empty\n+    end\n+\n+    it \"removes element\" do\n+      subject.add_click_listener(swt, click_block)\n+      subject.add_release_listener(swt, release_block)\n+\n+      subject.remove_listeners_for(dsl)\n+\n+      expect(subject.clickable_elements).to be_empty\n+    end\n+  end\n+\n+  describe \"event handling\" do\n+    before do\n+      subject.add_click_listener(swt, click_block)\n+      subject.add_release_listener(swt, release_block)\n+\n+      allow(dsl).to receive(:in_bounds?) { false }\n+      allow(dsl).to receive(:in_bounds?).with(10, 10) { true }\n+    end\n+\n+    shared_examples_for \"mouse event\" do\n+      it \"triggers\" do\n+        event = double(type: event_type, x: 10, y: 10)\n+        subject.handle_event(event)\n+\n+        expect(block).to have_received(:call)\n+      end\n+\n+      it \"doesn't trigger other block\" do\n+        event = double(type: event_type, x: 10, y: 10)\n+        subject.handle_event(event)\n+\n+        expect(other_block).to_not have_received(:call)\n+      end\n+\n+      it \"doesn't trigger click out of bounds\" do\n+        event = double(type: event_type, x: 20, y: 10)\n+        subject.handle_event(event)\n+\n+        expect(block).to_not have_received(:call)\n+      end\n+\n+      it \"re-registering an element's click overwrites old one\" do\n+        another_block = double(\"another block\", call: nil)\n+        subject.send(add_method, swt, another_block)\n+\n+        event = double(type: event_type, x: 10, y: 10)\n+        subject.handle_event(event)\n+\n+        expect(block).to_not     have_received(:call)\n+        expect(another_block).to have_received(:call)\n+      end\n+\n+      it \"takes the last element to respond\" do","path":"shoes-swt/spec/shoes/swt/click_listener_spec.rb","position":102,"original_position":102,"commit_id":"b28889c0e2d6611eb9b3caa362c1386173201701","original_commit_id":"b28889c0e2d6611eb9b3caa362c1386173201701","user":{"login":"PragTob","id":606517,"avatar_url":"https://avatars.githubusercontent.com/u/606517?v=3","gravatar_id":"","url":"https://api.github.com/users/PragTob","html_url":"https://github.com/PragTob","followers_url":"https://api.github.com/users/PragTob/followers","following_url":"https://api.github.com/users/PragTob/following{/other_user}","gists_url":"https://api.github.com/users/PragTob/gists{/gist_id}","starred_url":"https://api.github.com/users/PragTob/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/PragTob/subscriptions","organizations_url":"https://api.github.com/users/PragTob/orgs","repos_url":"https://api.github.com/users/PragTob/repos","events_url":"https://api.github.com/users/PragTob/events{/privacy}","received_events_url":"https://api.github.com/users/PragTob/received_events","type":"User","site_admin":false},"body":"I guess that's the answer to my last question, but I believe that relies on `hash.to_a` to return the elements in the order in which they were inserted and didn't that behavior change sometime?","created_at":"2014-12-16T12:04:02Z","updated_at":"2014-12-16T12:04:02Z","html_url":"https://github.com/shoes/shoes4/pull/980#discussion_r21892972","pull_request_url":"https://api.github.com/repos/shoes/shoes4/pulls/980","_links":{"self":{"href":"https://api.github.com/repos/shoes/shoes4/pulls/comments/21892972"},"html":{"href":"https://github.com/shoes/shoes4/pull/980#discussion_r21892972"},"pull_request":{"href":"https://api.github.com/repos/shoes/shoes4/pulls/980"}}},"pull_request":{"url":"https://api.github.com/repos/shoes/shoes4/pulls/980","id":26054040,"html_url":"https://github.com/shoes/shoes4/pull/980","diff_url":"https://github.com/shoes/shoes4/pull/980.diff","patch_url":"https://github.com/shoes/shoes4/pull/980.patch","issue_url":"https://api.github.com/repos/shoes/shoes4/issues/980","number":980,"state":"open","locked":false,"title":"Switch to app-level click listener","user":{"login":"jasonrclark","id":130504,"avatar_url":"https://avatars.githubusercontent.com/u/130504?v=3","gravatar_id":"","url":"https://api.github.com/users/jasonrclark","html_url":"https://github.com/jasonrclark","followers_url":"https://api.github.com/users/jasonrclark/followers","following_url":"https://api.github.com/users/jasonrclark/following{/other_user}","gists_url":"https://api.github.com/users/jasonrclark/gists{/gist_id}","starred_url":"https://api.github.com/users/jasonrclark/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jasonrclark/subscriptions","organizations_url":"https://api.github.com/users/jasonrclark/orgs","repos_url":"https://api.github.com/users/jasonrclark/repos","events_url":"https://api.github.com/users/jasonrclark/events{/privacy}","received_events_url":"https://api.github.com/users/jasonrclark/received_events","type":"User","site_admin":false},"body":"Couldn't resist after realizing what was happening on #882. Here's my shot at fixing the problem with events getting delivered when we didn't want them (specifically in that case, elements that were created in other click handlers).\r\n\r\nThe basic course I took was based on realizing we didn't need every element creating its own SWT-level listener. Instead, the window has one listener, and we do the element selection and dispatch ourselves. This gives us final control over who receives the event (in this case the final subscriber that's in bounds... maybe later we can get smarter like Shoes 3 where the top-most visible element gets it).\r\n\r\nThis deserves more testing, but some basics and links (which were the more complicated case for click support) are all working from what I've checked tonight (and specs were passing too).\r\n\r\n:mouse: :white_check_mark: :boom: \r\n\r\nThings to check\r\n* [ ] Run through samples, especially any that have click behavior\r\n* [ ] See if we can get rid of any further oddities around how links set up their clicks now that things are simpler\r\n* [ ] Should the shared `\"swt app\"` context have a `click_listener` set up automatically? Had several spots where I popped in a do-nothing version for tests to run.","created_at":"2014-12-15T08:46:46Z","updated_at":"2014-12-16T12:04:02Z","closed_at":null,"merged_at":null,"merge_commit_sha":"b6e6f0035ace1256d98c99a70d9269843fdc4507","assignee":null,"milestone":null,"commits_url":"https://api.github.com/repos/shoes/shoes4/pulls/980/commits","review_comments_url":"https://api.github.com/repos/shoes/shoes4/pulls/980/comments","review_comment_url":"https://api.github.com/repos/shoes/shoes4/pulls/comments/{number}","comments_url":"https://api.github.com/repos/shoes/shoes4/issues/980/comments","statuses_url":"https://api.github.com/repos/shoes/shoes4/statuses/b28889c0e2d6611eb9b3caa362c1386173201701","head":{"label":"shoes:one-click-listener","ref":"one-click-listener","sha":"b28889c0e2d6611eb9b3caa362c1386173201701","user":{"login":"shoes","id":119935,"avatar_url":"https://avatars.githubusercontent.com/u/119935?v=3","gravatar_id":"","url":"https://api.github.com/users/shoes","html_url":"https://github.com/shoes","followers_url":"https://api.github.com/users/shoes/followers","following_url":"https://api.github.com/users/shoes/following{/other_user}","gists_url":"https://api.github.com/users/shoes/gists{/gist_id}","starred_url":"https://api.github.com/users/shoes/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/shoes/subscriptions","organizations_url":"https://api.github.com/users/shoes/orgs","repos_url":"https://api.github.com/users/shoes/repos","events_url":"https://api.github.com/users/shoes/events{/privacy}","received_events_url":"https://api.github.com/users/shoes/received_events","type":"Organization","site_admin":false},"repo":{"id":4434126,"name":"shoes4","full_name":"shoes/shoes4","owner":{"login":"shoes","id":119935,"avatar_url":"https://avatars.githubusercontent.com/u/119935?v=3","gravatar_id":"","url":"https://api.github.com/users/shoes","html_url":"https://github.com/shoes","followers_url":"https://api.github.com/users/shoes/followers","following_url":"https://api.github.com/users/shoes/following{/other_user}","gists_url":"https://api.github.com/users/shoes/gists{/gist_id}","starred_url":"https://api.github.com/users/shoes/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/shoes/subscriptions","organizations_url":"https://api.github.com/users/shoes/orgs","repos_url":"https://api.github.com/users/shoes/repos","events_url":"https://api.github.com/users/shoes/events{/privacy}","received_events_url":"https://api.github.com/users/shoes/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/shoes/shoes4","description":"Shoes 4 : the next version of Shoes","fork":false,"url":"https://api.github.com/repos/shoes/shoes4","forks_url":"https://api.github.com/repos/shoes/shoes4/forks","keys_url":"https://api.github.com/repos/shoes/shoes4/keys{/key_id}","collaborators_url":"https://api.github.com/repos/shoes/shoes4/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/shoes/shoes4/teams","hooks_url":"https://api.github.com/repos/shoes/shoes4/hooks","issue_events_url":"https://api.github.com/repos/shoes/shoes4/issues/events{/number}","events_url":"https://api.github.com/repos/shoes/shoes4/events","assignees_url":"https://api.github.com/repos/shoes/shoes4/assignees{/user}","branches_url":"https://api.github.com/repos/shoes/shoes4/branches{/branch}","tags_url":"https://api.github.com/repos/shoes/shoes4/tags","blobs_url":"https://api.github.com/repos/shoes/shoes4/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/shoes/shoes4/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/shoes/shoes4/git/refs{/sha}","trees_url":"https://api.github.com/repos/shoes/shoes4/git/trees{/sha}","statuses_url":"https://api.github.com/repos/shoes/shoes4/statuses/{sha}","languages_url":"https://api.github.com/repos/shoes/shoes4/languages","stargazers_url":"https://api.github.com/repos/shoes/shoes4/stargazers","contributors_url":"https://api.github.com/repos/shoes/shoes4/contributors","subscribers_url":"https://api.github.com/repos/shoes/shoes4/subscribers","subscription_url":"https://api.github.com/repos/shoes/shoes4/subscription","commits_url":"https://api.github.com/repos/shoes/shoes4/commits{/sha}","git_commits_url":"https://api.github.com/repos/shoes/shoes4/git/commits{/sha}","comments_url":"https://api.github.com/repos/shoes/shoes4/comments{/number}","issue_comment_url":"https://api.github.com/repos/shoes/shoes4/issues/comments/{number}","contents_url":"https://api.github.com/repos/shoes/shoes4/contents/{+path}","compare_url":"https://api.github.com/repos/shoes/shoes4/compare/{base}...{head}","merges_url":"https://api.github.com/repos/shoes/shoes4/merges","archive_url":"https://api.github.com/repos/shoes/shoes4/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/shoes/shoes4/downloads","issues_url":"https://api.github.com/repos/shoes/shoes4/issues{/number}","pulls_url":"https://api.github.com/repos/shoes/shoes4/pulls{/number}","milestones_url":"https://api.github.com/repos/shoes/shoes4/milestones{/number}","notifications_url":"https://api.github.com/repos/shoes/shoes4/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/shoes/shoes4/labels{/name}","releases_url":"https://api.github.com/repos/shoes/shoes4/releases{/id}","created_at":"2012-05-24T14:57:59Z","updated_at":"2014-12-15T16:44:05Z","pushed_at":"2014-12-15T08:40:26Z","git_url":"git://github.com/shoes/shoes4.git","ssh_url":"git@github.com:shoes/shoes4.git","clone_url":"https://github.com/shoes/shoes4.git","svn_url":"https://github.com/shoes/shoes4","homepage":"","size":60060,"stargazers_count":703,"watchers_count":703,"language":"Ruby","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":136,"mirror_url":null,"open_issues_count":153,"forks":136,"open_issues":153,"watchers":703,"default_branch":"master"}},"base":{"label":"shoes:master","ref":"master","sha":"b34bc0a0c0cb419d0a7904b772765b4fff837258","user":{"login":"shoes","id":119935,"avatar_url":"https://avatars.githubusercontent.com/u/119935?v=3","gravatar_id":"","url":"https://api.github.com/users/shoes","html_url":"https://github.com/shoes","followers_url":"https://api.github.com/users/shoes/followers","following_url":"https://api.github.com/users/shoes/following{/other_user}","gists_url":"https://api.github.com/users/shoes/gists{/gist_id}","starred_url":"https://api.github.com/users/shoes/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/shoes/subscriptions","organizations_url":"https://api.github.com/users/shoes/orgs","repos_url":"https://api.github.com/users/shoes/repos","events_url":"https://api.github.com/users/shoes/events{/privacy}","received_events_url":"https://api.github.com/users/shoes/received_events","type":"Organization","site_admin":false},"repo":{"id":4434126,"name":"shoes4","full_name":"shoes/shoes4","owner":{"login":"shoes","id":119935,"avatar_url":"https://avatars.githubusercontent.com/u/119935?v=3","gravatar_id":"","url":"https://api.github.com/users/shoes","html_url":"https://github.com/shoes","followers_url":"https://api.github.com/users/shoes/followers","following_url":"https://api.github.com/users/shoes/following{/other_user}","gists_url":"https://api.github.com/users/shoes/gists{/gist_id}","starred_url":"https://api.github.com/users/shoes/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/shoes/subscriptions","organizations_url":"https://api.github.com/users/shoes/orgs","repos_url":"https://api.github.com/users/shoes/repos","events_url":"https://api.github.com/users/shoes/events{/privacy}","received_events_url":"https://api.github.com/users/shoes/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/shoes/shoes4","description":"Shoes 4 : the next version of Shoes","fork":false,"url":"https://api.github.com/repos/shoes/shoes4","forks_url":"https://api.github.com/repos/shoes/shoes4/forks","keys_url":"https://api.github.com/repos/shoes/shoes4/keys{/key_id}","collaborators_url":"https://api.github.com/repos/shoes/shoes4/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/shoes/shoes4/teams","hooks_url":"https://api.github.com/repos/shoes/shoes4/hooks","issue_events_url":"https://api.github.com/repos/shoes/shoes4/issues/events{/number}","events_url":"https://api.github.com/repos/shoes/shoes4/events","assignees_url":"https://api.github.com/repos/shoes/shoes4/assignees{/user}","branches_url":"https://api.github.com/repos/shoes/shoes4/branches{/branch}","tags_url":"https://api.github.com/repos/shoes/shoes4/tags","blobs_url":"https://api.github.com/repos/shoes/shoes4/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/shoes/shoes4/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/shoes/shoes4/git/refs{/sha}","trees_url":"https://api.github.com/repos/shoes/shoes4/git/trees{/sha}","statuses_url":"https://api.github.com/repos/shoes/shoes4/statuses/{sha}","languages_url":"https://api.github.com/repos/shoes/shoes4/languages","stargazers_url":"https://api.github.com/repos/shoes/shoes4/stargazers","contributors_url":"https://api.github.com/repos/shoes/shoes4/contributors","subscribers_url":"https://api.github.com/repos/shoes/shoes4/subscribers","subscription_url":"https://api.github.com/repos/shoes/shoes4/subscription","commits_url":"https://api.github.com/repos/shoes/shoes4/commits{/sha}","git_commits_url":"https://api.github.com/repos/shoes/shoes4/git/commits{/sha}","comments_url":"https://api.github.com/repos/shoes/shoes4/comments{/number}","issue_comment_url":"https://api.github.com/repos/shoes/shoes4/issues/comments/{number}","contents_url":"https://api.github.com/repos/shoes/shoes4/contents/{+path}","compare_url":"https://api.github.com/repos/shoes/shoes4/compare/{base}...{head}","merges_url":"https://api.github.com/repos/shoes/shoes4/merges","archive_url":"https://api.github.com/repos/shoes/shoes4/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/shoes/shoes4/downloads","issues_url":"https://api.github.com/repos/shoes/shoes4/issues{/number}","pulls_url":"https://api.github.com/repos/shoes/shoes4/pulls{/number}","milestones_url":"https://api.github.com/repos/shoes/shoes4/milestones{/number}","notifications_url":"https://api.github.com/repos/shoes/shoes4/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/shoes/shoes4/labels{/name}","releases_url":"https://api.github.com/repos/shoes/shoes4/releases{/id}","created_at":"2012-05-24T14:57:59Z","updated_at":"2014-12-15T16:44:05Z","pushed_at":"2014-12-15T08:40:26Z","git_url":"git://github.com/shoes/shoes4.git","ssh_url":"git@github.com:shoes/shoes4.git","clone_url":"https://github.com/shoes/shoes4.git","svn_url":"https://github.com/shoes/shoes4","homepage":"","size":60060,"stargazers_count":703,"watchers_count":703,"language":"Ruby","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":136,"mirror_url":null,"open_issues_count":153,"forks":136,"open_issues":153,"watchers":703,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/shoes/shoes4/pulls/980"},"html":{"href":"https://github.com/shoes/shoes4/pull/980"},"issue":{"href":"https://api.github.com/repos/shoes/shoes4/issues/980"},"comments":{"href":"https://api.github.com/repos/shoes/shoes4/issues/980/comments"},"review_comments":{"href":"https://api.github.com/repos/shoes/shoes4/pulls/980/comments"},"review_comment":{"href":"https://api.github.com/repos/shoes/shoes4/pulls/comments/{number}"},"commits":{"href":"https://api.github.com/repos/shoes/shoes4/pulls/980/commits"},"statuses":{"href":"https://api.github.com/repos/shoes/shoes4/statuses/b28889c0e2d6611eb9b3caa362c1386173201701"}}}}