xmonad / xmonad-contrib

Contributed modules for xmonad
https://xmonad.org
BSD 3-Clause "New" or "Revised" License
589 stars 274 forks source link

rewrite GridSelect.stringToRatio to use randomR (fixes #572) #573

Closed Rogach closed 3 years ago

Rogach commented 3 years ago

Description

Due to differences between random-1.1 and random-1.2, on newer systems stringToRatio returns numbers outside [0, 1] range, which breaks colorRangeFromClassName colorizers.

This commit fixes the issue by using randomR to directly generate the random number.

Also this fixes the compilation warning (genRange and next are deprecated in random-1.2).

Instead of using sum of charactes I've elected use a simple hash - this will result in a bit less collisions (simple sums of characters collide quite often).

TheMC47 commented 3 years ago

Thank you for your contribution :tada:

Also this fixes the compilation warning (genRange and next are deprecated in random-1.2).

Where can you see the logs, exactly? We tried to fix any warnings, and as far as I'm aware, we did (at least I don't see any while compiling locally or in the CI)

Rogach commented 3 years ago

@TheMC47 When compiling locally on my system (Arch Linux). Here's the link to the deprecated method: https://hackage.haskell.org/package/random-1.2.0/docs/System-Random.html#v:next

And here's the error message as seen on my system (building using runhaskell Setup.lhs build):

XMonad/Actions/GridSelect.hs:636:52: warning: [-Wdeprecations]
    In the use of ‘genRange’
    (imported from System.Random, but defined in System.Random.Internal):
    Deprecated: "No longer used"
    |
636 |                       range = (\(a, b) -> b - a) $ genRange gen
    |                                                    ^^^^^^^^

XMonad/Actions/GridSelect.hs:637:65: warning: [-Wdeprecations]
    In the use of ‘next’
    (imported from System.Random, but defined in System.Random.Internal):
    Deprecated: "No longer used"
    |
637 |                       randomInt = foldr1 combine $ replicate 20 next
    |                                                                 ^^^^
Rogach commented 3 years ago

@TheMC47 Separate commit in a separate pull request, or just another commit in this PR?

TheMC47 commented 3 years ago

@TheMC47 Separate commit in a separate pull request, or just another commit in this PR?

I'd say another PR, but I couldn't reproduce the warnings with a simple stack test. So maybe (again) wait for someone to chime in.

Ping @slotThe you seemed to have talked about this in the mailing list, so your feedback is appreciated! (I can't see your mails for some reason, they're also not in the archives)

Rogach commented 3 years ago

I'd say another PR, but I couldn't reproduce the warnings with a simple stack test

Did you run the test on this PR, but with the cabal changes reverted? If you just run this on the master branch then there shouldn't be any warnings - the warnings appeared due to the fact that I added an additional test file which pulled in lots of other modules (that's why I included the changes into this PR, by the way - the change seems conceptually related, to fix the warning in the same commit that would have introduced it).

TheMC47 commented 3 years ago

Yes I can confirm that's the case. Nevermind my comment then. Awesome!

slotThe commented 3 years ago

Thanks!