@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge
phorge phabricator

When inlines would disable a file shield in a diff, still apply the shield if all the comments are collapsed

Summary:
Ref T13515. We "shield" some changesets, including generated code and intradiffs with no intermediate changes.

These files don't get shielded if they have inline comments.

But, if the viewer has collapsed all the comments, we can shield the file again.

Test Plan:
- Created a change affecting files A and B, with three diffs:
- Touch A and B.
- Touch B only.
- Touch nothing.
- Added an inline to A and collapsed it.
- Viewed Diff 1 vs Diff 2:
- Saw A collapse with a note about inlines.
- Saw B changes, normally.
- Viewed Diff 2 vs Diff 3:
- Saw A collapse with a note about inlines.
- Saw B collapse normally.
- Uncollapsed the inline, viewed 1v2 and 2v3, saw A expand in both cases.

Maniphest Tasks: T13515

Differential Revision: https://secure.phabricator.com/D21136

+59 -18
+59 -18
src/applications/differential/parser/DifferentialChangesetParser.php
··· 879 879 880 880 $has_document_engine = ($engine_blocks !== null); 881 881 882 - $shield = null; 883 - if ($this->isTopLevel && !$this->comments && !$has_document_engine) { 882 + // See T13515. Sometimes, we collapse file content by default: for 883 + // example, if the file is marked as containing generated code. 884 + 885 + // If a file has inline comments, that normally means we never collapse 886 + // it. However, if the viewer has already collapsed all of the inlines, 887 + // it's fine to collapse the file. 888 + 889 + $expanded_comments = array(); 890 + foreach ($this->comments as $comment) { 891 + if ($comment->isHidden()) { 892 + continue; 893 + } 894 + $expanded_comments[] = $comment; 895 + } 896 + 897 + $collapsed_count = (count($this->comments) - count($expanded_comments)); 898 + 899 + $shield_raw = null; 900 + $shield_text = null; 901 + $shield_type = null; 902 + if ($this->isTopLevel && !$expanded_comments && !$has_document_engine) { 884 903 if ($this->isGenerated()) { 885 - $shield = $renderer->renderShield( 886 - pht( 887 - 'This file contains generated code, which does not normally '. 888 - 'need to be reviewed.')); 904 + $shield_text = pht( 905 + 'This file contains generated code, which does not normally '. 906 + 'need to be reviewed.'); 889 907 } else if ($this->isMoveAway()) { 890 908 // We put an empty shield on these files. Normally, they do not have 891 909 // any diff content anyway. However, if they come through `arc`, they ··· 895 913 // We could show a message like "this file was moved", but we show 896 914 // that as a change header anyway, so it would be redundant. Instead, 897 915 // just render an empty shield to skip rendering the diff body. 898 - $shield = ''; 916 + $shield_raw = ''; 899 917 } else if ($this->isUnchanged()) { 900 918 $type = 'text'; 901 919 if (!$rows) { ··· 909 927 $type = 'none'; 910 928 } 911 929 930 + $shield_type = $type; 931 + 912 932 $type_add = DifferentialChangeType::TYPE_ADD; 913 933 if ($this->changeset->getChangeType() == $type_add) { 914 934 // Although the generic message is sort of accurate in a technical 915 935 // sense, this more-tailored message is less confusing. 916 - $shield = $renderer->renderShield( 917 - pht('This is an empty file.'), 918 - $type); 936 + $shield_text = pht('This is an empty file.'); 919 937 } else { 920 - $shield = $renderer->renderShield( 921 - pht('The contents of this file were not changed.'), 922 - $type); 938 + $shield_text = pht('The contents of this file were not changed.'); 923 939 } 924 940 } else if ($this->isDeleted()) { 925 - $shield = $renderer->renderShield( 926 - pht('This file was completely deleted.')); 941 + $shield_text = pht('This file was completely deleted.'); 927 942 } else if ($this->changeset->getAffectedLineCount() > 2500) { 928 - $shield = $renderer->renderShield( 943 + $shield_text = pht( 944 + 'This file has a very large number of changes (%s lines).', 945 + new PhutilNumber($this->changeset->getAffectedLineCount())); 946 + } 947 + } 948 + 949 + $shield = null; 950 + if ($shield_raw !== null) { 951 + $shield = $shield_raw; 952 + } else if ($shield_text !== null) { 953 + if ($shield_type === null) { 954 + $shield_type = 'default'; 955 + } 956 + 957 + // If we have inlines and the shield would normally show the whole file, 958 + // downgrade it to show only text around the inlines. 959 + if ($collapsed_count) { 960 + if ($shield_type === 'text') { 961 + $shield_type = 'default'; 962 + } 963 + 964 + $shield_text = array( 965 + $shield_text, 966 + ' ', 929 967 pht( 930 - 'This file has a very large number of changes (%s lines).', 931 - new PhutilNumber($this->changeset->getAffectedLineCount()))); 968 + 'This file has %d collapsed inline comment(s).', 969 + new PhutilNumber($collapsed_count)), 970 + ); 932 971 } 972 + 973 + $shield = $renderer->renderShield($shield_text, $shield_type); 933 974 } 934 975 935 976 if ($shield !== null) {