...coming from #1628044: Implement magic for the table of attachments on issues and #1545922: [META] Issue page redesign

If I got things right, we'll be using this module in the new D7 d.o for listing all files posted in an issue. There's this preview dev site (use drupal/drupal for credentials), but I'm posting a screenshot here anyways just in case the site is taken down.

You'll notice how files from the same issue are listed without being grouped together based on the comment id to which they are attached to. This makes it harder in this example use case to visually associate each interdiff with its corresponding patch file unless you look at the "Comment ID" column searching for matching comment numbers.

I propose that we (optionally perhaps) allow files attached to the same comment to be grouped together. I propose we do that by merging the "Comment ID" table cells that contain the same comment id number.

group by comment id

Comments

Project:Attached Files Metadata Table» Project issue tracking
Version:7.x-1.x-dev» 7.x-2.x-dev
Component:Code» Comments

I tend to agree ... however, the comment ID's (and associated jumplinks) are added to the table by project_issue, and are not native to the file_metadata_table module itself; so any logic used to group the rows together based on comment_id probably belongs there as well.

The file_metadata_table module has an alter() hook which can be used to adjust the structure of the $rows array; so we could add a project_issue_file_metadata_table_output_alter() call which groups the rows by comment id as needed. Therefore, I'm going to move this over to the project_issue queue.

However, another item to consider is that we currently haven't yet implemented a 'show/hide files' capability when editing an issue. Once this is in place, the intended workflow would be to hide patches from view as they are rendered obsolete by subsequent patches in an issue thread. As a result, if users abide by this workflow, the table of files should never approach the length that is illustrated on the dev site ... the file_metadata_table formatter already supports hiding the non-relevant files in a secondary table located within a collapsed fieldset; which will significantly improve the experience relative to the dev site example.

Issue tags:+project, +Drupal.org D7

Priority:Normal» Minor

I don't think we need this for launch. I'm not even totally sure it's a good idea. For example, we want separate rows in this table for each patch uploaded, even if they're in the same comment, so that the test bot can do its thing in peace. Maybe there's some other grouping beyond trying to smoosh everything into the same row. Maybe we can play tricks with special-casing interdiff, but if we're going to do that, I'd rather just implement functionality on the site that tries to automatically generate interdiffs (I don't remember seeing a feature request about that, but someone might find something via searching -- if so, feel free to x-link it here).

Anyway, we can leave this tagged for the d.o D7 port, but I'm downgrading to minor make it clear this isn't a blocker. #1833684: Need a file widget that gracefully handles lots of hidden files (which resolves #1245508: Issue summaries: Allow outdated attached files to be hidden/replaced/flagged.) is much more urgent.

Thanks,
-Derek

...we currently haven't yet implemented a 'show/hide files' capability ... Once this is in place, ... the table of files should never approach the length that is illustrated on the dev site...

True (I was the one to file #1245508: Issue summaries: Allow outdated attached files to be hidden/replaced/flagged.), but if one clicks the "show all files" link (to view the file history for example), they'd still face the visual mess.

Also when hiding files, we'd have to be smart and make sure that we don't simply rely on limiting the number of files listed because that could cause hiding only some of the files posted in a comment: ...say we limit the number of files shown to a hard-codded 5 and there are initially 4 files uploaded. Then another user uploads another 3 files causing the visible part of the table to show the 3 new files + 2 from the previous 4. Would there be a visual indication that another 2 files were uploaded along with the previous comment? or would we use the hard-codded number of files to be shown more like a threshold and show the files in batches (per comment that they were attached to)? This wouldn't be an issue if files from a single comment were grouped together in a single "consolidated" table row.

Then again, what about hiding only files that render other files obsolete? If we did group files together per comment, then how do we "remove" obsolete files from their group?

Maybe we can play tricks with special-casing interdiff, ...

Besides having a patch and a diff, comments can have 3, 4 or even more screenshots + a patch and a diff. The general idea is to group all these files from a single comment together. I initially thought that we could use a single row and stick all files in a <li>, but then reconsidered because that would make it really hard to color-code testbot results :/

As for obsoleting files, grouping patch and diff pairs is a good thing (if a patch is obsolete, then so is its diff). So, I guess that patches and respective diffs should be paired.

...but if we're going to do that, I'd rather just implement functionality on the site that tries to automatically generate interdiffs...

I was thinking about the auto-generation of interdiffs just last night when I saw again a one too many comment from a person requesting an interdiff from another one that had just uploaded a patch. I thought how tedious and frustrating this whole thing must have been for both of them: the one to have to remind the other about the interdiff and the other to have the review process of their patch halted because of such a trivial thing when they have put so much energy on the actual job of creating the patch. Hence...

@klonos: I really appreciate your interest in all of these problems. However, I'm going to firmly request that you carefully read the current plan before writing more comments like this. ;) I don't want to keep explaining why your concerns are either misplaced or already addressed with the approach we're in the middle of implementing.

Thanks,
-Derek

p.s. That was slightly harsh, sorry. "why your concerns are either misplaced or already addressed" should say: "why most of your concerns..."

Anyway, the basic point is still true. :) For now, I'm going to spend my time implementing the current plan and then you can tell us if/why it's not sufficient once you actually see it working.

Thanks/sorry,
-Derek

To respond to #4, the file_metadata_table keys off of the file 'status' flag to determine whether the file should be displayed or not. Thus, while editing an issue, a user would flag any existing files which are obsoleted by the patch currently being uploaded (via a custom widget, still to be developed).

Thus, the display doesn't truncate based solely on number of files ... it would be up to users to prune the display.

...your concerns are either misplaced or already addressed with the approach we're in the middle of implementing.

...you can tell us if/why it's not sufficient once you actually see it working.

Fair enough. I'll wait ;)

I didn't mean to be judgmental or anything. I just wanted to keep my concerns documented in a single place for future reference. That's all.

Title:Group together files posted in the same revision.Group together files posted in the same comment.
Project:Extended File Field» Project issue tracking
Version:7.x-1.x-dev» 7.x-2.x-dev
Component:User interface» Comments

This issue goes beyond just patch + interdiff file pairs (such as for example screenshots attached along with a patch), but when it comes to patch + interdiff, if we do implement #1956166: Auto-generate interdiffs between patch files (from patches uploaded previously). I think it would be great to "hide" interdiffs in a "diff" operation link. Look at how bugzilla does it:

diff operation link available in .patch filesAlso should we switch this to Attached Files Metadata Table?

Title:Group together files posted in the same comment.Group together files posted in the same revision.
Project:Project issue tracking» Attached Files Metadata Table
Version:7.x-2.x-dev» 7.x-1.x-dev
Component:Comments» User interface

Yes, moving to a better queue.

Related:
#1962930: Make the file upload widget as flexible as the display formatter: alter hook, configurable table columns
#1961712: Notices and bugs when files are deleted without a new node revision.

I'm tempted to remove the d.o D7 tag from this, but so long as we leave this minor/feature request I think we're okay.

p.s. I don't see what screenshot #10 has to do with this feature request. Sounds like you're talking about either #1956166: Auto-generate interdiffs between patch files (from patches uploaded previously). or the hidden files stuff that's already done and working. I'm not seeing how that helps with grouping files that were uploaded with the same node revision which is what this issue is about.

Since the user and autogenerated comment # will be the same, perhaps we really want a row for each node revision where we list all the shared info once (including perhaps a timestamp), with a nested row for each file attached in that revision. A mockup of that would be useful. We haven't done any mockup revisions in a long time, and I think a final pass with the UX team before launch would be important.

Thanks,
-Derek

While I initially punted this to project_issue (based on us not having the cid), I can support this as a setting which allows grouping of files which were added in the same revision.

However, any implementation for drupal.org is going to have to consider how PIFT integrates to provide patch testing status ... so I'd like to wait until after that is implemented before tackling this, so as to not paint ourselves into a difficult corner.

Some mockups to help visualize the final product with full PIFT integration might help bring me around, so I won't straight-up postpone this. :)

Project:Attached Files Metadata Table» Extended File Field

Title:Group together files posted in the same comment.Group together files posted in the same revision.
Project:Project issue tracking» Extended File Field
Version:7.x-2.x-dev»
Component:Comments» User interface
StatusFileSize
new8.55 KB

@dww, #12:

I don't see what screenshot #10 has to do with this feature request. ...

...attaching a better screenie for #10.

Were we planing to add an "Operations" column to the table anyways? I don't see anything in the mockups from #1545922: [META] Issue page redesign, but we do have #1964926: Rename hooks from "metadata" to "columns".

Version:» 7.x-1.x-dev

Yes there's an operations column.

As I've said, the mockups aren't final, haven't been looked at in a long time, and need to be updated again soon as we're getting close to something that's viable for launch.

Great! Hope the new screenshot in #10 now makes sense.

Status:Active» Closed (won't fix)

This has been implemented within pift_extended_file_field_output_alter(), which is an alter hook exposed by the Extended File Field module. See #1979574: Testbot info display on issues for the resulting output.

This should give us what we need on drupal.org, so I'm marking it as "won't fix" for now; in lieu of a more effective solution being implemented elsewhere.

For the reasons outlined in comment #1, I don't think I want to pursue this further within the extended_file_field module itself, as the 'comment id' is not native to this module, and the grouping of files added in the same revision feels like a very minor edge case ...

That said, if you feel strongly that the grouping of files from the same revision into the same row would still be a valuable feature within the Extended File Field module itself, feel free to re-open it.

Project:Extended File Field» Project issue tracking
Version:7.x-1.x-dev» 7.x-2.x-dev
Status:Closed (won't fix)» Active

...I still find it distracting/annoying while scanning through file lists (especially long ones). Not high priority, but would be great to get it fixed. So, switching to project_issue if you say it's more appropriate for comment number handling. The reason I filed this request against Extended File Field in the first place was because it concerned the file table.

Another thing to consider is that extended_file_field allows you to hide files from that list ... so for the intended drupal.org workflow, only the most current/relevant patches should appear in the file table; and ideally, we shouldn't have many long file listings.

Priority:Minor» Normal
Issue tags:-project, -Drupal.org D7+Drupal.org 7.1

This will have to wait until after launch unless someone volunteers to work on it.

I'm wondering if the current operation may be good enough, with the zebra striping identifying which files were added in which revision. klonos, can you look at git7site.devdrupal.org and comment on whether the distinction is enough (so that we can decide whether to close this issue)?

The zebra grouping based on issue number seems a step towards the right direction. Still this is true only for us that know about this. If I look through the eyes of someone accustomed to the odd/even zebra stripping, then it takes me a second to realize that the numbers in the first column are the same for each stripe. Once I realize that, then the next obvious question that comes to mind is why not to merge the cells with the same numbers.

...in other words, yeah I like the zebra stripping logic, but still think that the issue number cells should be merged. Anyways, that's my personal opinion and I do hate the fact that there's only 4 of us following this issue. I'd love it if we could get some more eyes on this and some additional feedback so I can be sure that it's not just me ;)

Issue summary:View changes

...created an img tag from the uploaded file.

Untagging, let Dev Tools review this and decide if it's needed or not.

Project:Project issue tracking» Project issue file test
Priority:Normal» Minor
Issue tags:-Needs DSWG Dev Tools Team feedback+D.o UX

This looks doable within pift_extended_file_field_output_alter(), which is where the striping is implemented. Instead of hard-coding the rowspan to 2, it will have to be variable.

Since there is the striping, moving down to minor priority.