Randomize placement of project slider

Inventitech commented 6 years ago

This issue consists of the following substeps:

[x] Add slider back for Eclipse
[x] Add slider back for IntelliJ
[x] Randomize slider position
[x] Transfer slider position

Inventitech commented 6 years ago

@TimvdLippe would you be willing to review this once I am done?

TimvdLippe commented 6 years ago

I can do static reviews, but I can not run the code.

Inventitech commented 6 years ago

Ok, cool, thx. I'll ping you once done.

Inventitech commented 6 years ago

Done. @TimvdLippe, would appreciate your feedback on this one.

Pretty flabbergasted by the number of issues I came across while implementing this TBH (#320, #319, and additionally commits 8b29992, 11fda2b).

TimvdLippe commented 6 years ago

320 not sure what went wrong here. The original PR does shows that the code was correct initially: https://github.com/TestRoots/watchdog/pull/294/files#diff-bbb5f27114924e48bb328054682d8818R96 I have no clue how this did not end up in master.
319 is described in point 7 of https://github.com/TestRoots/watchdog/blob/master/CONTRIBUTING.md#intellij and which was supposed to be further clarified in #309 per our discussion back then. However, that PR was never merged.
https://github.com/TestRoots/watchdog/commit/8b29992c77de48fe4a2bfef6f942b01cf3712bc6 while exceptions rarely happen, I never ran into this issue, nor did we get any report for that. Interested to know in which situation you were encountering this bug. Nice fix though 😄

I do not share your flabbergasted reaction, as all of these issues either occur in very rare cases or were already documented/clarified. Given the large amount of changes that went live, we had a release with a very low aftermath of bugs. I am quite pleased with that result personally.

I will review your code probably on monday, as I will be busy this weekend and not have a laptop available to me.

TimvdLippe commented 6 years ago

@Inventitech I can't find a PR for this issue. Did I miss it? Would like to have a place where I can post the review comments 😄

Inventitech commented 6 years ago

Hi Tim,

Yap, sorry, I didn't think of the possibility to get a review on this at first, so committed directly to master. I'll see if I can at least get a decent diff.

Will get back.

Inventitech commented 6 years ago

One comment to your first bullet point: The code does indeed work in Eclipse (as stated in #320), the link you posted points to the (correct) code in Eclipse. In the same PR, we can also see the erroneous lines of code in the code clone for IntelliJ.

I created #321 so you can review the changes. Thanks!

TestRoots / watchdog

Randomize placement of project slider #318

320 not sure what went wrong here. The original PR does shows that the code was correct initially: https://github.com/TestRoots/watchdog/pull/294/files#diff-bbb5f27114924e48bb328054682d8818R96 I have no clue how this did not end up in `master`.

319 is described in point 7 of https://github.com/TestRoots/watchdog/blob/master/CONTRIBUTING.md#intellij and which was supposed to be further clarified in #309 per our discussion back then. However, that PR was never merged.

TestRoots / watchdog

Randomize placement of project slider #318

320 not sure what went wrong here. The original PR does shows that the code was correct initially: https://github.com/TestRoots/watchdog/pull/294/files#diff-bbb5f27114924e48bb328054682d8818R96 I have no clue how this did not end up in master.

319 is described in point 7 of https://github.com/TestRoots/watchdog/blob/master/CONTRIBUTING.md#intellij and which was supposed to be further clarified in #309 per our discussion back then. However, that PR was never merged.

320 not sure what went wrong here. The original PR does shows that the code was correct initially: https://github.com/TestRoots/watchdog/pull/294/files#diff-bbb5f27114924e48bb328054682d8818R96 I have no clue how this did not end up in `master`.