Page MenuHomePhabricator

Fix some more setIcon() issues
ClosedPublic

Authored by epriestley on Aug 31 2016, 4:51 PM.

Details

Summary

Fixes T11569. This fixes a known bad setIcon(). I also looked for more calls to setIcon() without success, and stubbed setIcon() so we're in good shape even if more exist.

Test Plan
  • Grepped for setIcon( and manually inspect all 1,004 callsites to look for calls on PHUIObjectItemView objects.
  • Grepped for "high risk" callsites (setIcon in file after PHUIObjectItemView) and re-examined them. I identified these files with this command:
git ls-tree -r --name-only HEAD | xargs pcregrep -i -M -H -c --files-with-matches -o 'PHUIObjectItemView(.|\n)*setIcon'

There might be some more clever way to do that.

  • Since this only identified the callsites I already knew about and I don't have a ton of confidence that I didn't miss any, I put a stub in place that logs a deprecation warning. I'll file a followup to go clean these up in a month or so if the logs are clean.
  • Loaded Nuance, saw it work but warn.
  • Changed Nuance to use setStatusIcon(), loaded Nuance, no more fatal.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

epriestley retitled this revision from to Fix some more setIcon() issues.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: chad.

These should be setImageIcon, which is the big icon. StatusIcon is the little icon.

epriestley edited edge metadata.

Ah, thanks!

  • Use setImageIcon() instead of setStatusIcon().
  • Advise setImageIcon() in deprecation warning, and call through to setImageIcon() instead.

Tested by:

  • Reverting to setIcon().
  • Loading page, verifying it works with large icons but with warnings.
  • Changing to setImageIcon().
  • Loading page, works with large icons and no warnings.
chad edited edge metadata.

I think I named it as "icon, the size of an image"

This revision is now accepted and ready to land.Aug 31 2016, 4:58 PM

Yeah, it's a little weird but sort of makes sense considering stuff like the People list and Projects list where we have actual images. I don't know that setLargeIcon() or whatever would really be any more clear.

This revision was automatically updated to reflect the committed changes.