Edgewall Software
Modify

Opened 14 years ago

Last modified 8 months ago

#5549 new defect

Improve diff message "no files"

Reported by: Emmanuel Blot Owned by:
Priority: normal Milestone: next-stable-1.4.x
Component: version control/changeset view Version: devel
Severity: normal Keywords:
Cc: Branch:
Release Notes:
API Changes:
Internal Changes:

Description

When a changeset contains only minor modifications (blank line, whitespace changes …) and the matching "ignore" checkboxes are selected, Trac outputs a confusing message:

(No files)

whereas the actual message should be something like "(No visible changes)", as the changeset does contain files, but the selected diff options lead to an empty diff.

This is confusing especially as Trac conveniently keeps track of the user diff preferences from one diff to another - which implies that a user can get a "(No files)" message as soon as he wants a diff.

Attachments (1)

minimal-hidden_properties-diff-rendering.patch (1011 bytes ) - added by Christian Boos 13 years ago.
Show at least Property xyz changed for a change of a hidden property, so that there's at least one file entry in the Files: list.

Download all attachments as: .zip

Change History (21)

comment:1 by sid, 13 years ago

Keywords: patch added

Simple patch to fix this issue:

  • trac/versioncontrol/templates/changeset.html

     
    135135          <dt class="property location">Location:</dt>
    136136          <dd class="searchable"><a href="${req.href.browser(location, rev=new_rev)}">$location</a></dd>
    137137        </py:if>
    138         <dt class="property files">${files and 'Files:' or '(No files)'}</dt>
     138        <dt class="property files">${files and 'Files:' or '(No visible changes)'}</dt>
    139139        <dd class="files">
    140140          <div class="legend" id="file-legend" py:if="filestats">
    141141            <dl>

comment:2 by Christian Boos, 13 years ago

Milestone: 0.11.1

comment:3 by osimons, 13 years ago

Any reason why this cannot just be committed? The new text provides good information, both for whitespace and hidden properties that can make a changset appear completely empty.

comment:4 by Christian Boos, 13 years ago

Milestone: 0.11.10.11

That change is fine for me, we can commit it.

comment:5 by osimons, 13 years ago

Actually, that did not turn out so nice with regards to formatting. There is only room for 1-2 words, so it breaks a line and just looks a bit out of place on the side of the page.

Alternative proposal:

  • trac/versioncontrol/templates/changeset.html

     
    135135          <dt class="property location">Location:</dt>
    136136          <dd class="searchable"><a href="${req.href.browser(location, rev=new_rev)}">$location</a></dd>
    137137        </py:if>
    138         <dt class="property files">${files and 'Files:' or '(No files)'}</dt>
     138        <dt class="property files">Files:</dt>
    139139        <dd class="files">
     140          <py:if test="not files">No files with visible changes.</py:if>
    140141          <div class="legend" id="file-legend" py:if="filestats">
    141142            <dl>
    142143              <py:if test="filestats['add']"><dt class="add"></dt><dd>${filestats['add']} added</dd></py:if>

I basically renders:

Files: No files with visible changes.

That will also give better space for future l10n messages that might need more room.

Only question: Any reason why it should not render correctly being placed inside the <dd> tag? Any circumstance where it will not be alone, or why we can't have just text instead of the ususal lists?

comment:6 by Christian Boos, 13 years ago

That latest change is better. Still, I think that for "perfect" clarity we should make clear whether the list of files is empty because there's no files in the changeset or no visible change according to the current diff options.

I'm OK for committing the intermediate improvement of comment:5, but maybe leave the ticket open for 0.11.1 as a reminder to implement the above.

comment:7 by osimons, 13 years ago

When testing this, I found that even property changes rendered as a file. I was not able to think of anything (using Subversion) that could make a change without listing some file - ie. in all circumstances it was diff options that made this list appear.

Perhaps with other versioncontrol systems? Could there be circumstances with full diff that as change is made that has no file references? The versioncontrol code is really not what I know best…

comment:8 by Christian Boos, 13 years ago

Well, the situations I had in mind for the no files were admittedly corner cases.

The first was when a property change is hidden on purpose ([browser] hide_properties), e.g. r5705.

The second situation was root property change. But in this case we generate a pseudo-"(root)" file entry.

So the only confusing situation would be r5705, as there's no way a change of diff options will make that prop change visible.

I'll attach a patch shortly that would instead render this minimal information about the hidden property change:

  • i18n

    • Property svnmerge-integrated changed

That will give some hint about what's going on in changesets like r5705, while preserving the uncluttered view that hide_properties was meant to provide.

by Christian Boos, 13 years ago

Show at least Property xyz changed for a change of a hidden property, so that there's at least one file entry in the Files: list.

comment:9 by Christian Boos, 13 years ago

Ah, and of course, besides the above corner cases that are now addressed, there's one more situation to think about: changesets restricted on a path outside of the scope of the changeset, e.g. [6334/sandbox]. In this case, it's a legitimate (No files).

comment:10 by anonymous, 13 years ago

are there also changesets that could be empty due to permission settings, ie, you don't have permission to see the changed files?

in reply to:  10 comment:11 by osimons, 13 years ago

Replying to anonymous:

are there also changesets that could be empty due to permission settings, ie, you don't have permission to see the changed files?

Yes. Both regular permissions, and I Just configured a project today myself where anonymous has CHANGESET_VIEW for seeing changesets in Timeline and reading log messages, but no FILE_VEW permissions to see the changed files. That will actually list (No files.) for all changesets.

comment:12 by Christian Boos, 13 years ago

Milestone: 0.110.12
Severity: minornormal

This issue is less trivial than it seemed at first. I think we should keep the (No files) for now and improve that message once with the correct information later on:

  • "(No files)" when this is effectively a changeset without files (i.e. property changes only)
  • "(No files in the restricted scope)" when there are files in the changeset but outside of the restricted path
  • "(No visible changes)" when there should be files but there's no changes to show because of the current diff options
  • more?

in reply to:  12 comment:13 by Emmanuel Blot, 12 years ago

Replying to cboos:

This issue is less trivial than it seemed at first. I think we should keep the (No files) for now and improve that message once with the correct information later on:

The current implementation is unsettling for the end user.
If a changeset only contains property change(s):

  • the diff view shows "(No files)" - and nothing more, while
  • the changeset view shows the actual property change (with the property name and value)

comment:14 by Christian Boos, 12 years ago

Milestone: 0.130.12

comment:15 by Remy Blank, 11 years ago

Milestone: 0.12next-minor-0.12.x

comment:16 by Christian Boos, 10 years ago

Keywords: patch removed

Removing keyword, as there's no patch currently meeting the requirements.

comment:17 by Ryan J Ollos, 6 years ago

Milestone: next-minor-0.12.xnext-stable-1.0.x

comment:18 by Ryan J Ollos, 6 years ago

Owner: Christian Boos removed

comment:19 by Ryan J Ollos, 4 years ago

Milestone: next-stable-1.0.xnext-stable-1.2.x

Moved ticket assigned to next-stable-1.0.x since maintenance of 1.0.x is coming to a close. Please move the ticket back if it's critical to fix on 1.0.x.

comment:20 by Ryan J Ollos, 8 months ago

Milestone: next-stable-1.2.xnext-stable-1.4.x

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The ticket will remain with no owner.
The ticket will be disowned.
as The resolution will be set. Next status will be 'closed'.
The owner will be changed from (none) to anonymous. Next status will be 'assigned'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.