Update notification.lua #1075

Merged
merged 1 commit into from Jan 11, 2016

Projects

None yet

7 participants

@OmegaExtern
Contributor

Fixes overlapping bug in AddProgress function.

@OmegaExtern OmegaExtern Update notification.lua
Fixes overlapping bug in AddProgress function.
fa28f13
@PotcFdk
Contributor
PotcFdk commented Oct 13, 2015

I object to this bugfix plus whitespacing-adjustment nightmare.

@Kefta
Contributor
Kefta commented Oct 13, 2015

@PotcFdk Why?

@OmegaExtern
Contributor

@PotcFdk Reason?
Anyway, think again.

First:
notification.AddProgress( "SomeID", "Some text" )
https://i.imgur.com/dZ9hRAs.png

Second:
notification.AddProgress( "AnotherID", "Another text" )
https://i.imgur.com/rSnxcn9.png

Third:
notification.AddProgress( "SomeID", "New text" )
https://i.imgur.com/NFVZIll.png

Also, without this bug-fix it is impossible to get rid off the first notification (using notification.Kill), because the first notification gets replaced by the third notification.
And yes, this patch also formats notification.lua file with a style that is suggested on Garry's Mod wikipedia (notice it is also used widely in this repository).
This fixes overlapping by checking if there is already a valid notification with the specified identifier, if so just update text. This check was somehow left out, probably during copy+paste from older GMod version..

@PotcFdk
Contributor
PotcFdk commented Oct 17, 2015

@OmegaExtern Because this is an ugly PR. All your changes are in one huge commit, including whitespacing changes. It's really hard and annoying to find out what is going on.

http://blog.ploeh.dk/2015/01/15/10-tips-for-better-pull-requests/

@Kefta
Contributor
Kefta commented Oct 17, 2015

@PotcFdk You can easily tell the changes, and less commits in this case is fine for a chained change like this. The white space can be fixed afterwards if the author doesn't fix it himself

@Kefta
Contributor
Kefta commented Oct 17, 2015

A change shouldn't be discouraged because of a few cosmetic things that can easily be fixed by anyone afterward

@LennyPenny
Contributor

no because then we'd have a cleanup commit after every contributed commit lol

@Kefta
Contributor
Kefta commented Oct 18, 2015

It's just one commit; this hasn't been an issue in the past

@GGG-KILLER

"Oh, this code does not follow my coding style standards, let's just reject it because all these spaces are confusing me"
If it adds a functionality, it works while not breaking any current script and the code can be understood, then why not?
All these whitespaces in fact made the code more readable in my opinion.
I vote 👍 for this pr

@OmegaExtern OmegaExtern commented on the diff Jan 5, 2016
garrysmod/lua/includes/modules/notification.lua
local w = self:GetWide() * 0.25
local x = math.fmod( SysTime() * 200, self:GetWide() + w ) - w
-
- if ( x + w > self:GetWide() - 11 ) then w = ( self:GetWide() - 11 ) - x end
@OmegaExtern
OmegaExtern Jan 5, 2016 Contributor

Removed a whitespace at the end of the line - not entire line.

@OmegaExtern OmegaExtern commented on the diff Jan 5, 2016
garrysmod/lua/includes/modules/notification.lua
local dist = ideal_y - y
Panel.VelY = Panel.VelY + dist * spd * 1
- if (math.abs(dist) < 2 && math.abs(Panel.VelY) < 0.1) then Panel.VelY = 0 end
- local dist = ideal_x - x
@OmegaExtern
OmegaExtern Jan 5, 2016 Contributor

Notice "dist" is already defined as local variable above.

@OmegaExtern OmegaExtern commented on the diff Jan 5, 2016
garrysmod/lua/includes/modules/notification.lua
@@ -79,45 +89,40 @@ local function UpdateNotice( i, Panel, Count )
local x = Panel.fx
local y = Panel.fy
-
- local w = Panel:GetWide()
- local h = Panel:GetTall()
-
- w = w + 16
- h = h + 16
@OmegaExtern
OmegaExtern Jan 5, 2016 Contributor

Added on initialization line (same for "w").

@OmegaExtern OmegaExtern commented on the diff Jan 5, 2016
garrysmod/lua/includes/modules/notification.lua
@@ -25,6 +25,16 @@ local Notices = {}
function AddProgress( uid, text )
+ if ( IsValid( Notices[ uid ] ) ) then
@OmegaExtern
OmegaExtern Jan 5, 2016 Contributor

Actual bug fix.

@PotcFdk
Contributor
PotcFdk commented Jan 5, 2016

I'm not saying that changes to the whitespacing shouldn't happen in any case. But jesus, please, at least split them into their own commit so the owners/contributors can cherry-pick if necessary.
I'm not trying to just put blames into this PR - it's just that the use of Git comes with some common knowledge / Dos and Donts.

@GGG-KILLER

then why not?

See #1075 (comment):

It's really hard and annoying to find out what is going on.

@Bo98
Contributor
Bo98 commented Jan 5, 2016

Regarding the:

It's really hard and annoying to find out what is going on.

That's solvable with the ?w=1 URL parameter:

https://github.com/garrynewman/garrysmod/pull/1075/files?w=1

@PotcFdk
Contributor
PotcFdk commented Jan 5, 2016

That isn't available in Git, or just outside of GitHub at all - so it's an ugly service-dependant solution to an unnecessary issue. Stop avoiding the fact.

@GGG-KILLER

That isn't available in Git, or just outside of GitHub at all - so it's an ugly service-dependant solution to an unnecessary issue. Stop avoiding the fact.

This is GitHub and yes, we will use GitHub-specific solutions.
Proper use of git was never required for pull requests, so just stop complaining about the way he did the pull request and instead give some actual feedback on what matters: the code
We're not avoiding the fact, we're ignoring it because it is not important.

@robotboy655 robotboy655 merged commit f459739 into garrynewman:master Jan 11, 2016
@OmegaExtern OmegaExtern deleted the OmegaExtern:patch-1 branch Jan 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment