Page MenuHomePhabricator

Differential - stop showing the shield for "move away" operations
ClosedPublic

Authored by btrahan on Nov 17 2014, 10:20 PM.
Tags
None
Referenced Files
F14036383: D10865.id.diff
Sun, Nov 10, 10:02 AM
F14035679: D10865.id.diff
Sun, Nov 10, 7:07 AM
F14015594: D10865.diff
Sun, Nov 3, 9:52 PM
F13994519: D10865.id26089.diff
Wed, Oct 23, 6:43 AM
F13992581: D10865.id26092.diff
Tue, Oct 22, 5:33 PM
F13975712: D10865.diff
Fri, Oct 18, 11:19 AM
F13955437: D10865.diff
Mon, Oct 14, 1:25 AM
Unknown Object (File)
Oct 9 2024, 1:18 PM
Subscribers

Details

Summary

The shield is just confusing. In one case it doesn't work, and in the other case it just shows you a copy of the file you can see just below except in red. Fixes T4599, T1211. Note T1211 proposed not showing the "move away" file at all but I think removing the shield fixes the source of confusion. The code here is a bit if / else if / else if... heavy but this is logically sound.

Test Plan

made a diff where i moved a file then edited it in the new location. viewed diff, saw confusing shield, dropped caches, applied patch, viewed diff and saw no shield. made a diff where I moved a file and didn't edit in new location and saw similar shield disappearness.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

btrahan retitled this revision from to Differential - stop showing the shield for "move away" operations.
btrahan updated this object.
btrahan edited the test plan for this revision. (Show Details)
btrahan added a reviewer: epriestley.
src/applications/differential/parser/DifferentialChangesetParser.php
86

i could purge all caches but thought that was excessive? probably should

epriestley edited edge metadata.

I think leaving the caches is reasonable. This is pretty rare and this is the most expensive cache to rebuild.

I wonder if we should always shield these with a message like "This file was moved elsewhere." or "This file was moved to <destination link>"? We'll basically always show the whole file in red now, right? And show it again elsewhere in the diff? Or am I getting the behavior wrong?

This is less confusing/incorrect and strictly an improvement, in any case.

This revision is now accepted and ready to land.Nov 17 2014, 10:32 PM

Screen_Shot_2014-11-17_at_2.47.50_PM.png (1×2 px, 344 KB)

Here's a very mature screenshot of the new behavior. If I hadn't edited the file in the new location, there would be a shield there to show the contents as they were unchanged.

This revision was automatically updated to reflect the committed changes.