GitHub PullRequestReviewCommentEvent at 2014-08-04 07:53:16
Repository: CraftBukkit
Actor: turt2live
Payload: {"action":"created","comment":{"url":"https://api.github.com/repos/Bukkit/CraftBukkit/pulls/comments/15743997","id":15743997,"diff_hunk":"@@ -19,9 +21,18 @@ public CraftThrownPotion(CraftServer server, EntityPotion entity) {\n         super(server, entity);\n     }\n \n-    // TODO: This one does not handle custom NBT potion effects does it?\n-    // In that case this method could be said to be misleading or incorrect\n     public Collection<PotionEffect> getEffects() {\n+        // We need to check if there are custom effects first\n+        if (getItem().hasItemMeta()) {\n+        ItemMeta itemMeta = getItem().getItemMeta();\n+            if (itemMeta instanceof PotionMeta) {\n+                PotionMeta potionMeta = (PotionMeta) itemMeta;\n+                if (potionMeta.hasCustomEffects()) {\n+                    // With custom effects present, Minecraft ignores damage value\n+                    return potionMeta.getCustomEffects();\n+                }\n+            }\n+\t\t}","path":"src/main/java/org/bukkit/craftbukkit/entity/CraftThrownPotion.java","position":26,"original_position":26,"commit_id":"f2d39cabc4d5d8d77f671c20f92dff3dff685a38","original_commit_id":"f2d39cabc4d5d8d77f671c20f92dff3dff685a38","user":{"login":"turt2live","id":1190097,"avatar_url":"https://avatars.githubusercontent.com/u/1190097?v=1","gravatar_id":"1ff897313ab7ed45a24871913d0968e6","url":"https://api.github.com/users/turt2live","html_url":"https://github.com/turt2live","followers_url":"https://api.github.com/users/turt2live/followers","following_url":"https://api.github.com/users/turt2live/following{/other_user}","gists_url":"https://api.github.com/users/turt2live/gists{/gist_id}","starred_url":"https://api.github.com/users/turt2live/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/turt2live/subscriptions","organizations_url":"https://api.github.com/users/turt2live/orgs","repos_url":"https://api.github.com/users/turt2live/repos","events_url":"https://api.github.com/users/turt2live/events{/privacy}","received_events_url":"https://api.github.com/users/turt2live/received_events","type":"User","site_admin":false},"body":"Tabs are evil.","created_at":"2014-08-04T07:53:16Z","updated_at":"2014-08-04T07:53:16Z","html_url":"https://github.com/Bukkit/CraftBukkit/pull/1401#discussion_r15743997","pull_request_url":"https://api.github.com/repos/Bukkit/CraftBukkit/pulls/1401","_links":{"self":{"href":"https://api.github.com/repos/Bukkit/CraftBukkit/pulls/comments/15743997"},"html":{"href":"https://github.com/Bukkit/CraftBukkit/pull/1401#discussion_r15743997"},"pull_request":{"href":"https://api.github.com/repos/Bukkit/CraftBukkit/pulls/1401"}}},"pull_request":{"url":"https://api.github.com/repos/Bukkit/CraftBukkit/pulls/1401","id":18948414,"html_url":"https://github.com/Bukkit/CraftBukkit/pull/1401","diff_url":"https://github.com/Bukkit/CraftBukkit/pull/1401.diff","patch_url":"https://github.com/Bukkit/CraftBukkit/pull/1401.patch","issue_url":"https://api.github.com/repos/Bukkit/CraftBukkit/issues/1401","number":1401,"state":"open","title":"[C] Check for custom potion effects before falling back on damage value. Fixes BUKKIT-3635","user":{"login":"LB--","id":1296838,"avatar_url":"https://avatars.githubusercontent.com/u/1296838?v=1","gravatar_id":"cbe910169ce9db2330f1a998d206aa85","url":"https://api.github.com/users/LB--","html_url":"https://github.com/LB--","followers_url":"https://api.github.com/users/LB--/followers","following_url":"https://api.github.com/users/LB--/following{/other_user}","gists_url":"https://api.github.com/users/LB--/gists{/gist_id}","starred_url":"https://api.github.com/users/LB--/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/LB--/subscriptions","organizations_url":"https://api.github.com/users/LB--/orgs","repos_url":"https://api.github.com/users/LB--/repos","events_url":"https://api.github.com/users/LB--/events{/privacy}","received_events_url":"https://api.github.com/users/LB--/received_events","type":"User","site_admin":false},"body":"**The Issue:**\r\nThe collection of `PotionEffect`s returned by `CraftThrownPotion#getEffects()` only considered the damage value of the potion. There was a TODO comment above the implementation explaining that this was incorrect behavior. This PR fixes the implementation to have the intended behavior so that `CraftThrownPotion#getEffects()` returns the same collection of `PotionEffect`s that vanilla Minecraft actually applies to players.\r\n\r\n**Justification for this PR:**\r\nSince `CraftThrownPotion#getEffects()` could potentially be a useful convenience method to developers, I think it would be better to repair it than to deprecate it or leave it as it is. I have personally been mislead by it with its current implementation, and the JIRA ticket is proof other developers have been mislead too. With the changes in this PR, it saves some extra work a plugin developer has to do to get the desired collection of potion effects.\r\n\r\n**PR Breakdown:**\r\nFirst, the new implementation takes the `ItemMeta` from the `ItemStack` returned by `CraftThrownPotion#getItem()`. If the returned value is not null and is an instance of `PotionMeta`, it grabs the collection of `PotionEffect`s returned by `PotionMeta#getCustomEffects()`. If this collection is non-empty, it is returned since Minecraft will ignore the data value. If, however, it *is* empty, it falls back on the previous implementation of using damage value to acquire the collection of `PotionEffect`s, since vanilla Minecraft will be using the damage value as well.\r\n\r\n**Testing Results and Materials:**\r\nI tested the old and new implementation with this test plugin:\r\nhttps://github.com/LB--/TestPlugin/tree/BUKKIT-3635\r\nUpon login, the plugin gives you a splash potion with damage value for slowness but custom effect of speed 2. It listens to `PotionSplashEvent` and displays to the thrower the contents of `ThrownPotion#getEffects()` as well as `PotionMeta#getCustomEffects()`. Before this commit, the former displayed only the slowness effect and the latter displayed only the speed 2 effect. After this commit, both the former and the latter display the speed 2 effect. In both cases, the effect applied to the player was the speed 2 effect.\r\n\r\n**JIRA Ticket:**\r\nBUKKIT-3635 - https://bukkit.atlassian.net/browse/BUKKIT-3635","created_at":"2014-07-28T03:22:57Z","updated_at":"2014-08-04T07:53:16Z","closed_at":null,"merged_at":null,"merge_commit_sha":"7bf55a097fb52aee274cf57b8b71a8b4cf2904c1","assignee":null,"milestone":null,"commits_url":"https://api.github.com/repos/Bukkit/CraftBukkit/pulls/1401/commits","review_comments_url":"https://api.github.com/repos/Bukkit/CraftBukkit/pulls/1401/comments","review_comment_url":"https://api.github.com/repos/Bukkit/CraftBukkit/pulls/comments/{number}","comments_url":"https://api.github.com/repos/Bukkit/CraftBukkit/issues/1401/comments","statuses_url":"https://api.github.com/repos/Bukkit/CraftBukkit/statuses/f2d39cabc4d5d8d77f671c20f92dff3dff685a38","head":{"label":"LB--:BUKKIT-3635","ref":"BUKKIT-3635","sha":"f2d39cabc4d5d8d77f671c20f92dff3dff685a38","user":{"login":"LB--","id":1296838,"avatar_url":"https://avatars.githubusercontent.com/u/1296838?v=1","gravatar_id":"cbe910169ce9db2330f1a998d206aa85","url":"https://api.github.com/users/LB--","html_url":"https://github.com/LB--","followers_url":"https://api.github.com/users/LB--/followers","following_url":"https://api.github.com/users/LB--/following{/other_user}","gists_url":"https://api.github.com/users/LB--/gists{/gist_id}","starred_url":"https://api.github.com/users/LB--/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/LB--/subscriptions","organizations_url":"https://api.github.com/users/LB--/orgs","repos_url":"https://api.github.com/users/LB--/repos","events_url":"https://api.github.com/users/LB--/events{/privacy}","received_events_url":"https://api.github.com/users/LB--/received_events","type":"User","site_admin":false},"repo":{"id":22328442,"name":"CraftBukkit","full_name":"LB--/CraftBukkit","owner":{"login":"LB--","id":1296838,"avatar_url":"https://avatars.githubusercontent.com/u/1296838?v=1","gravatar_id":"cbe910169ce9db2330f1a998d206aa85","url":"https://api.github.com/users/LB--","html_url":"https://github.com/LB--","followers_url":"https://api.github.com/users/LB--/followers","following_url":"https://api.github.com/users/LB--/following{/other_user}","gists_url":"https://api.github.com/users/LB--/gists{/gist_id}","starred_url":"https://api.github.com/users/LB--/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/LB--/subscriptions","organizations_url":"https://api.github.com/users/LB--/orgs","repos_url":"https://api.github.com/users/LB--/repos","events_url":"https://api.github.com/users/LB--/events{/privacy}","received_events_url":"https://api.github.com/users/LB--/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/LB--/CraftBukkit","description":"The Minecraft Server Mod API Implementation","fork":true,"url":"https://api.github.com/repos/LB--/CraftBukkit","forks_url":"https://api.github.com/repos/LB--/CraftBukkit/forks","keys_url":"https://api.github.com/repos/LB--/CraftBukkit/keys{/key_id}","collaborators_url":"https://api.github.com/repos/LB--/CraftBukkit/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/LB--/CraftBukkit/teams","hooks_url":"https://api.github.com/repos/LB--/CraftBukkit/hooks","issue_events_url":"https://api.github.com/repos/LB--/CraftBukkit/issues/events{/number}","events_url":"https://api.github.com/repos/LB--/CraftBukkit/events","assignees_url":"https://api.github.com/repos/LB--/CraftBukkit/assignees{/user}","branches_url":"https://api.github.com/repos/LB--/CraftBukkit/branches{/branch}","tags_url":"https://api.github.com/repos/LB--/CraftBukkit/tags","blobs_url":"https://api.github.com/repos/LB--/CraftBukkit/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/LB--/CraftBukkit/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/LB--/CraftBukkit/git/refs{/sha}","trees_url":"https://api.github.com/repos/LB--/CraftBukkit/git/trees{/sha}","statuses_url":"https://api.github.com/repos/LB--/CraftBukkit/statuses/{sha}","languages_url":"https://api.github.com/repos/LB--/CraftBukkit/languages","stargazers_url":"https://api.github.com/repos/LB--/CraftBukkit/stargazers","contributors_url":"https://api.github.com/repos/LB--/CraftBukkit/contributors","subscribers_url":"https://api.github.com/repos/LB--/CraftBukkit/subscribers","subscription_url":"https://api.github.com/repos/LB--/CraftBukkit/subscription","commits_url":"https://api.github.com/repos/LB--/CraftBukkit/commits{/sha}","git_commits_url":"https://api.github.com/repos/LB--/CraftBukkit/git/commits{/sha}","comments_url":"https://api.github.com/repos/LB--/CraftBukkit/comments{/number}","issue_comment_url":"https://api.github.com/repos/LB--/CraftBukkit/issues/comments/{number}","contents_url":"https://api.github.com/repos/LB--/CraftBukkit/contents/{+path}","compare_url":"https://api.github.com/repos/LB--/CraftBukkit/compare/{base}...{head}","merges_url":"https://api.github.com/repos/LB--/CraftBukkit/merges","archive_url":"https://api.github.com/repos/LB--/CraftBukkit/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/LB--/CraftBukkit/downloads","issues_url":"https://api.github.com/repos/LB--/CraftBukkit/issues{/number}","pulls_url":"https://api.github.com/repos/LB--/CraftBukkit/pulls{/number}","milestones_url":"https://api.github.com/repos/LB--/CraftBukkit/milestones{/number}","notifications_url":"https://api.github.com/repos/LB--/CraftBukkit/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/LB--/CraftBukkit/labels{/name}","releases_url":"https://api.github.com/repos/LB--/CraftBukkit/releases{/id}","created_at":"2014-07-28T02:36:04Z","updated_at":"2014-07-27T21:07:32Z","pushed_at":"2014-07-29T01:07:33Z","git_url":"git://github.com/LB--/CraftBukkit.git","ssh_url":"git@github.com:LB--/CraftBukkit.git","clone_url":"https://github.com/LB--/CraftBukkit.git","svn_url":"https://github.com/LB--/CraftBukkit","homepage":"http://bukkit.org","size":12650,"stargazers_count":0,"watchers_count":0,"language":null,"has_issues":false,"has_downloads":true,"has_wiki":false,"forks_count":0,"mirror_url":null,"open_issues_count":0,"forks":0,"open_issues":0,"watchers":0,"default_branch":"master"}},"base":{"label":"Bukkit:master","ref":"master","sha":"3833911d519c3ad3f1af6e9d77a48b9889ae0054","user":{"login":"Bukkit","id":544609,"avatar_url":"https://avatars.githubusercontent.com/u/544609?v=1","gravatar_id":"cafe4b8d296dd981f519b7789b9159de","url":"https://api.github.com/users/Bukkit","html_url":"https://github.com/Bukkit","followers_url":"https://api.github.com/users/Bukkit/followers","following_url":"https://api.github.com/users/Bukkit/following{/other_user}","gists_url":"https://api.github.com/users/Bukkit/gists{/gist_id}","starred_url":"https://api.github.com/users/Bukkit/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Bukkit/subscriptions","organizations_url":"https://api.github.com/users/Bukkit/orgs","repos_url":"https://api.github.com/users/Bukkit/repos","events_url":"https://api.github.com/users/Bukkit/events{/privacy}","received_events_url":"https://api.github.com/users/Bukkit/received_events","type":"Organization","site_admin":false},"repo":{"id":1215253,"name":"CraftBukkit","full_name":"Bukkit/CraftBukkit","owner":{"login":"Bukkit","id":544609,"avatar_url":"https://avatars.githubusercontent.com/u/544609?v=1","gravatar_id":"cafe4b8d296dd981f519b7789b9159de","url":"https://api.github.com/users/Bukkit","html_url":"https://github.com/Bukkit","followers_url":"https://api.github.com/users/Bukkit/followers","following_url":"https://api.github.com/users/Bukkit/following{/other_user}","gists_url":"https://api.github.com/users/Bukkit/gists{/gist_id}","starred_url":"https://api.github.com/users/Bukkit/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Bukkit/subscriptions","organizations_url":"https://api.github.com/users/Bukkit/orgs","repos_url":"https://api.github.com/users/Bukkit/repos","events_url":"https://api.github.com/users/Bukkit/events{/privacy}","received_events_url":"https://api.github.com/users/Bukkit/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/Bukkit/CraftBukkit","description":"The Minecraft Server Mod API Implementation","fork":false,"url":"https://api.github.com/repos/Bukkit/CraftBukkit","forks_url":"https://api.github.com/repos/Bukkit/CraftBukkit/forks","keys_url":"https://api.github.com/repos/Bukkit/CraftBukkit/keys{/key_id}","collaborators_url":"https://api.github.com/repos/Bukkit/CraftBukkit/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/Bukkit/CraftBukkit/teams","hooks_url":"https://api.github.com/repos/Bukkit/CraftBukkit/hooks","issue_events_url":"https://api.github.com/repos/Bukkit/CraftBukkit/issues/events{/number}","events_url":"https://api.github.com/repos/Bukkit/CraftBukkit/events","assignees_url":"https://api.github.com/repos/Bukkit/CraftBukkit/assignees{/user}","branches_url":"https://api.github.com/repos/Bukkit/CraftBukkit/branches{/branch}","tags_url":"https://api.github.com/repos/Bukkit/CraftBukkit/tags","blobs_url":"https://api.github.com/repos/Bukkit/CraftBukkit/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/Bukkit/CraftBukkit/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/Bukkit/CraftBukkit/git/refs{/sha}","trees_url":"https://api.github.com/repos/Bukkit/CraftBukkit/git/trees{/sha}","statuses_url":"https://api.github.com/repos/Bukkit/CraftBukkit/statuses/{sha}","languages_url":"https://api.github.com/repos/Bukkit/CraftBukkit/languages","stargazers_url":"https://api.github.com/repos/Bukkit/CraftBukkit/stargazers","contributors_url":"https://api.github.com/repos/Bukkit/CraftBukkit/contributors","subscribers_url":"https://api.github.com/repos/Bukkit/CraftBukkit/subscribers","subscription_url":"https://api.github.com/repos/Bukkit/CraftBukkit/subscription","commits_url":"https://api.github.com/repos/Bukkit/CraftBukkit/commits{/sha}","git_commits_url":"https://api.github.com/repos/Bukkit/CraftBukkit/git/commits{/sha}","comments_url":"https://api.github.com/repos/Bukkit/CraftBukkit/comments{/number}","issue_comment_url":"https://api.github.com/repos/Bukkit/CraftBukkit/issues/comments/{number}","contents_url":"https://api.github.com/repos/Bukkit/CraftBukkit/contents/{+path}","compare_url":"https://api.github.com/repos/Bukkit/CraftBukkit/compare/{base}...{head}","merges_url":"https://api.github.com/repos/Bukkit/CraftBukkit/merges","archive_url":"https://api.github.com/repos/Bukkit/CraftBukkit/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/Bukkit/CraftBukkit/downloads","issues_url":"https://api.github.com/repos/Bukkit/CraftBukkit/issues{/number}","pulls_url":"https://api.github.com/repos/Bukkit/CraftBukkit/pulls{/number}","milestones_url":"https://api.github.com/repos/Bukkit/CraftBukkit/milestones{/number}","notifications_url":"https://api.github.com/repos/Bukkit/CraftBukkit/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/Bukkit/CraftBukkit/labels{/name}","releases_url":"https://api.github.com/repos/Bukkit/CraftBukkit/releases{/id}","created_at":"2011-01-03T00:10:05Z","updated_at":"2014-08-03T13:24:08Z","pushed_at":"2014-08-02T21:12:27Z","git_url":"git://github.com/Bukkit/CraftBukkit.git","ssh_url":"git@github.com:Bukkit/CraftBukkit.git","clone_url":"https://github.com/Bukkit/CraftBukkit.git","svn_url":"https://github.com/Bukkit/CraftBukkit","homepage":"http://bukkit.org","size":32400,"stargazers_count":2091,"watchers_count":2091,"language":"Java","has_issues":false,"has_downloads":true,"has_wiki":false,"forks_count":1296,"mirror_url":null,"open_issues_count":81,"forks":1296,"open_issues":81,"watchers":2091,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/Bukkit/CraftBukkit/pulls/1401"},"html":{"href":"https://github.com/Bukkit/CraftBukkit/pull/1401"},"issue":{"href":"https://api.github.com/repos/Bukkit/CraftBukkit/issues/1401"},"comments":{"href":"https://api.github.com/repos/Bukkit/CraftBukkit/issues/1401/comments"},"review_comments":{"href":"https://api.github.com/repos/Bukkit/CraftBukkit/pulls/1401/comments"},"review_comment":{"href":"https://api.github.com/repos/Bukkit/CraftBukkit/pulls/comments/{number}"},"commits":{"href":"https://api.github.com/repos/Bukkit/CraftBukkit/pulls/1401/commits"},"statuses":{"href":"https://api.github.com/repos/Bukkit/CraftBukkit/statuses/f2d39cabc4d5d8d77f671c20f92dff3dff685a38"}}}}