i2p / i2p.i2p

I2P is an anonymizing network, offering a simple layer that identity-sensitive applications can use to securely communicate. All data is wrapped with several layers of encryption, and the network is both distributed and dynamic, with no trusted parties.
https://geti2p.net
Other
1.95k stars 305 forks source link

I2PSnark: Fix a [very] minor bug and "Modernize" folder.js #14

Closed PimpTrizkit closed 4 years ago

PimpTrizkit commented 6 years ago

1) When viewing the main info screen of a torrent in I2PSnark, if that torrent's root consists of all folders and no files, then the Priority buttons at the bottom will not be visible. However, somewhere in the code, I have not found where yet but it still calls "setupbuttons()" even tho there are no buttons to setup. Therefore, it crashes in the browser. A very inconsequential bug; as there is no more code to run afterwards. But who likes seeing red lines in the console. a) Simply fixed by softening "setupbuttons()" up a little, with a check for existence of the "savepri" button. So now, if the code accidentally throws a call to either of these two functions "setupbuttons()" and "priorityclicked()"... it won't crash. 2) While I was here I decided to also "Modernize" the code a little with the new-er JavaScript standard. LET declarations and the arrow functions are lighter-weight. Also, I trimmed up the FOR loops and IF statements. a) I quickened up the loops. unified variable declarations, cached out length and data array, got rid of post-expression in the FOR loop in favor of in-line, on an existing expression. b) For the "updatesetallbuttons()", I used Short-Circuit logic, once all three are found, no need to keep searching. I inverted the booleans around to make more sense (if notNorm is true.. then its true that we don't want the Norm button) and to avoid using nots ("!"). c) Trimmed nested IF statements into one with short circuit operator (&&)... (same pseudo logic). d) Removed some IF statements in favor of the more concise ternary operator (?:).... (same pseudo logic).

---- Or you can just ignore most all of this and just add an IF statement in "setupbuttons()" before calling "updatesetallbuttons()".

codecov-io commented 6 years ago

Codecov Report

Merging #14 into master will increase coverage by <.01%. The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #14      +/-   ##
============================================
+ Coverage      6.39%    6.39%   +<.01%     
+ Complexity     2264     2263       -1     
============================================
  Files          1236     1236              
  Lines        131098   131097       -1     
  Branches      25173    25173              
============================================
+ Hits           8385     8386       +1     
- Misses       121258   121260       +2     
+ Partials       1455     1451       -4
Impacted Files Coverage Δ Complexity Δ
...pps/susimail/src/src/i2p/susi/webmail/WebMail.java 0% <0%> (ø) 0 <0> (ø) :arrow_down:
core/java/src/net/i2p/crypto/DSAEngine.java 28.73% <0%> (-1.12%) 28% <0%> (-2%)
router/java/src/net/i2p/router/time/NtpClient.java 28.31% <0%> (-0.89%) 4% <0%> (-2%)
core/java/src/net/i2p/util/RandomSource.java 48.35% <0%> (+1.09%) 8% <0%> (+1%) :arrow_up:
core/java/src/net/i2p/crypto/ElGamalEngine.java 66.66% <0%> (+1.19%) 9% <0%> (+1%) :arrow_up:
...e/java/src/gnu/crypto/prng/BasePRNGStandalone.java 64.58% <0%> (+6.25%) 11% <0%> (+1%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 0ba0f1b...09387a7. Read the comment docs.

majestrate commented 6 years ago

looks good so far.

PimpTrizkit commented 6 years ago

Thanks. I'm relatively new to GitHub. Can anyone explain why the Oracle check failed? and whats this coverage thing? hehe

PT

PimpTrizkit commented 6 years ago

Also, as you can see I merged in all the commits from the master, to bring my fork up to date before I made my commits. But I didn't know that was going to show up here. Should I have not done that? and just send the pull with only my changes, even tho my fork was behind?

majestrate commented 6 years ago

the code will be merged via monotone, i2p uses monotone for source control, this is a git mirror of the monotone repo inside i2p. we should move to git eventually :+DDD

the check is from code that isn't "fixed" yet that exists in the project rn.

PimpTrizkit commented 6 years ago

Oh, thank you so much for the info! Ya, I was confused about the check... because my fork was exactly like the master and his seemed to pass the checks.

Ah, so you guys are planning to move to GitHub? Coolio, I was really regretting having to learn monotone... lol... I guess there is no timeline on that?

majestrate commented 6 years ago

i2p has been meaning to move to git for years (not github exclusively) it'll get done eventually.

PimpTrizkit commented 6 years ago

Ah, gotcha. So does this mean that when I committed in the updates from master before my updates... which now shows as an additional commit on my behalf.. which is wrong, because its zzz's update merged into mine... anyhow... having that extra commit that is not mine .. this is ok? because it doesn't really matter what happens on github, since you use monotone?

majestrate commented 6 years ago

the git / mtn interoperation is a work in progress

PimpTrizkit commented 6 years ago

hehe, I dont actually know what that means as far as an answer to my question... I assume the answer is ... yes... it doesn't matter if I update my fork with his commits before I do the pull request. (thus adding an additional commit to my pull request)

majestrate commented 6 years ago

this PR will be merged eventually via monotone. may take a few days.

PimpTrizkit commented 6 years ago

Yes and thank you for your time, you have been most helpful. But I understand this. I was just concerned that having this extra commit on my PR might mess with that merge thru monotone. I presume that it is of no concern.

holzingk commented 6 years ago

git migration is not on the project roadmap, although there is always some discussion. Easiest way to get this merged is to send a patch via https://trac.i2p2.de or will you do it @majestrate ?

majestrate commented 6 years ago

zzz ask me on irc to review this PR so i am assuming he'll merge it.

On Fri, Feb 16, 2018 at 07:40:15PM +0000, Kilian Holzinger wrote:

git migration is not on the project roadmap, although there is always some discussion. Easiest way to get this merged is to send a patch via https://trac.i2p2.de or will you do it @majestrate ?

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/i2p/i2p.i2p/pull/14#issuecomment-366337994

PimpTrizkit commented 6 years ago

I've already done that on trac. I was just concerned of extra clutter in this PR. It seems its no concern.

PimpTrizkit commented 6 years ago

Oh shoot! How do I do this? I have a new, unrelated, update for which I want to make a new PR. But before I got that far, I see my commits to my fork have been added to this PR. Is this right? Was I supposed to wait until this PR was cleared out? Or how do I go about this the right way?

Thanks!

PT

majestrate commented 6 years ago

We can close the PR once it's merged via mtn.

On Sat, Feb 24, 2018 at 12:32:00AM -0800, Pimp Trizkit wrote:

Oh shoot! How do I do this? I have a new, unrelated, update for which I want to make a new PR. But before I got that far, I see my commits to my fork have been added to this PR. Is this right? Was I supposed to wait until this PR was cleared out? Or how do I go about this the right way?

Thanks!

PT

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/i2p/i2p.i2p/pull/14#issuecomment-368211258

PimpTrizkit commented 6 years ago

Oh, ok. So it will sit around until then.

Well, how do I continue with my unrelated updates if they keep adding to this existing PR? (I don't want to stop developing because I'm waiting on a merger)

PT

majestrate commented 6 years ago

I'll ask zzz when he's arround today.

On Sat, Feb 24, 2018 at 03:56:07AM -0800, Pimp Trizkit wrote:

Oh, ok. So it will sit around until then.

Well, how do I continue with my unrelated updates if they keep adding to this existing PR? (I don't want to stop developing because I'm waiting on a merger)

PT

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/i2p/i2p.i2p/pull/14#issuecomment-368223358