Don't compare string with empty string
ClosedPublic

Authored by ognarb on Fri, May 15, 9:03 AM.

Details

Summary

Instead compare string length with an integer

Test Plan

Compile && run

Diff Detail

Repository
R108 KWin
Branch
string-compare (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 27087
Build 27105: arc lint + arc unit
ognarb created this revision.Fri, May 15, 9:03 AM
Restricted Application added a project: KWin. · View Herald TranscriptFri, May 15, 9:03 AM
Restricted Application added a subscriber: kwin. · View Herald Transcript
ognarb requested review of this revision.Fri, May 15, 9:03 AM
broulik added inline comments.
kcmkwin/kwinrules/package/contents/ui/FileDialogLoader.qml
39

Why not just !root.lastFolder? or even

root.lastFolder || shortcuts.home
ognarb planned changes to this revision.Fri, May 15, 9:37 AM
ognarb added inline comments.
kcmkwin/kwinrules/package/contents/ui/FileDialogLoader.qml
39

I didn't know that an empty string in js was returning false.

broulik added inline comments.Fri, May 15, 9:38 AM
kcmkwin/kwinrules/package/contents/ui/FileDialogLoader.qml
39

It's not, but the operators in JS work a little differently. They don't cast to bool.

ognarb updated this revision to Diff 82914.Fri, May 15, 9:58 AM
  • reflect comment
iasensio added inline comments.Fri, May 15, 10:00 AM
kcmkwin/kwinrules/package/contents/ui/FileDialogLoader.qml
39

I also didn't know that about JS. Python operators also behave that way. Nice!

ognarb updated this revision to Diff 82915.Fri, May 15, 10:04 AM

Found another one

All the changes seem nice but this one. I've not yet figure out why

kcmkwin/kwinrules/package/contents/ui/FileDialogLoader.qml
49

This seems to break importing files for me.
Checking lenght > 0 also didn't work, just old if (fileUrl != "").

broulik added inline comments.Fri, May 15, 2:54 PM
kcmkwin/kwinrules/package/contents/ui/FileDialogLoader.qml
39

There's also ?? operator in Qt 5.15 which is the proper Null coalescing operator

49

If fileUrl is a QUrl, then that can lead to funky results as url basic type isn't astring. So fileUrl != "" it is (not the strict operator).. or fileUrl.toString() !== ""

iasensio requested changes to this revision.Fri, May 29, 2:54 PM

Ping @ognarb? I think it's just the fileUrl thing that's missing.

This revision now requires changes to proceed.Fri, May 29, 2:54 PM

sorry forgot about it, let me finish it now

No rush, I just wanted to use Requested Changes for the first and last time here at Phab :D

ognarb updated this revision to Diff 83174.Fri, May 29, 6:50 PM

Revert fileUrl change

ognarb marked 2 inline comments as done.Fri, May 29, 6:51 PM
ognarb updated this revision to Diff 83176.Fri, May 29, 8:15 PM
ognarb marked an inline comment as done.

Rebase

iasensio accepted this revision.Fri, May 29, 8:44 PM

LGTM!

This revision is now accepted and ready to land.Fri, May 29, 8:44 PM
ognarb closed this revision.Fri, May 29, 8:50 PM