🎨 Enhanced Visual Diff System for Card Description Changes (Resolves #7201)#7212
🎨 Enhanced Visual Diff System for Card Description Changes (Resolves #7201)#7212LexioJ wants to merge 5 commits into
Conversation
LexioJ
requested review from
elzody,
grnd-alt and
luka-nextcloud
as code owners
LexioJ
force-pushed
the
feature/visual-diff-7201-v1.15.2
branch
2 times, most recently
from
f83b4b9 to
8b42605
Compare
|
Looks interesting. Does it also address #7058? |
Hard to tell as issue #7058 seems to discuss the overall appearance within the activity app and a wrong description notification when only comments were made. This PR indeed addresses the need to reduce bloating the activity log, but only within the activity tab of the corresponding card. It focuses on the deck app itself. |
LexioJ
force-pushed
the
feature/visual-diff-7201-v1.15.2
branch
7 times, most recently
from
b5f9e4b to
86df615
Compare
LexioJ
force-pushed
the
feature/visual-diff-7201-v1.15.2
branch
from
86df615 to
e8b1112
Compare
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
LexioJ
force-pushed
the
feature/visual-diff-7201-v1.15.2
branch
8 times, most recently
from
2a58b7c to
40315b9
Compare
LexioJ
force-pushed
the
feature/visual-diff-7201-v1.15.2
branch
from
40315b9 to
0f5ec86
Compare
luka-nextcloud
left a comment
luka-nextcloud
left a comment
There was a problem hiding this comment.
Thank you for your contribution. The line number seems incorrect.
| $this->l10nFactory = $l10n; | ||
| $this->config = $config; | ||
| $this->cardService = $cardService; | ||
| $this->diffService = new DiffService(); |
There was a problem hiding this comment.
The DiffService can be inject via the constructor instead of instantiating it directly
There was a problem hiding this comment.
Year in copyright text should be 2025
| */ | ||
| private function isCheckboxChange(string $oldLine, string $newLine): bool { | ||
| // Pattern for markdown checkboxes: - [ ] or - [x] or - [X] | ||
| $checkboxPattern = '/^(\s*-\s*)\[([ xX])\](.*)/i'; |
There was a problem hiding this comment.
The checkbox pattern can be defined as a constant to avoid duplicate code
| * @return string | ||
| */ | ||
| private function generateCheckboxDiff(string $oldLine, string $newLine): string { | ||
| $checkboxPattern = '/^(\s*-\s*)\[([ xX])\](.*)/i'; |
There was a problem hiding this comment.
The checkbox pattern can be defined as a constant to avoid duplicate code
| * @param array $newLines | ||
| * @return string | ||
| */ | ||
| private function enhanceWithWordLevelDiff(string $html, array $operations, array $oldLines, array $newLines): string { |
There was a problem hiding this comment.
The $html is not used anywhere
| } | ||
|
|
||
| // Add remaining unused remove operations (not involved in moves or modifications) | ||
| foreach ($removes as $removeIndex => $removeOp) { |
There was a problem hiding this comment.
The $removeIndex is not used
LexioJ
force-pushed
the
feature/visual-diff-7201-v1.15.2
branch
from
5cca5d9 to
bdd5fe0
Compare
LexioJ
force-pushed
the
feature/visual-diff-7201-v1.15.2
branch
2 times, most recently
from
314af8a to
57c59a5
Compare
LexioJ
force-pushed
the
feature/visual-diff-7201-v1.15.2
branch
3 times, most recently
from
c1c3510 to
420cc89
Compare
|
@LexioJ Let me know if this PR is ready for review. |
LexioJ
force-pushed
the
feature/visual-diff-7201-v1.15.2
branch
from
420cc89 to
afb4821
Compare
Actually it is, I addressed all your remarks, improved the code & style further and being excited if this proposal will find its way to Deck for imho everyone’s benefit |
| 'new_line' => $bestMatch['new_line'] | ||
| ]; | ||
| $usedAdds[] = $bestAddIndex; | ||
| $usedRemoves[] = $removeIndex; |
There was a problem hiding this comment.
It can be moved to outside of if statement.
There was a problem hiding this comment.
I am sorry but I am not able to add you recommendations as I no longer understand the branches in this repo - seems I messed up my fork and local repo. If you like to use/integrate this feature I ask the nextcloud team to adapt/use the code I contributed
| } else { | ||
| // No good match, keep as remove | ||
| $enhancedOps[] = $removeOp; | ||
| $usedRemoves[] = $removeIndex; |
There was a problem hiding this comment.
It can be moved to outside of if statement.
There was a problem hiding this comment.
I am sorry but I am not able to add you recommendations as I no longer understand the branches in this repo - seems I messed up my fork and local repo. If you like to use/integrate this feature I ask the nextcloud team to adapt/use the code I contributed
| case 'remove': | ||
| $word = $oldWords[$operation['old_line']] ?? ''; | ||
| break; | ||
| case 'keep': | ||
| $word = $oldWords[$operation['old_line']] ?? ''; | ||
| break; |
There was a problem hiding this comment.
Case remove and keep can be merged since they have same result
There was a problem hiding this comment.
I am sorry but I am not able to add you recommendations as I no longer understand the branches in this repo - seems I messed up my fork and local repo. If you like to use/integrate this feature I ask the nextcloud team to adapt/use the code I contributed
There was a problem hiding this comment.
Hey @LexioJ, thank you for your contribution! Would it be possible to show insertions and additions with strikethrough and highlighted style respectively?
A bit like:
In this reference from notion, "cleaning" was deleted and the "shower" was added
LexioJ
force-pushed
the
feature/visual-diff-7201-v1.15.2
branch
from
afb4821 to
4381b10
Compare
It was my origin intent. But then I realized the limitations given by the activity app (which is responsible for rendering). I am using ins and del tags which underline or strikes-out on most common browsers. |
This commit introduces a sophisticated visual diff system that enhances the activity feed with intelligent change detection and professional presentation. Resolves issue nextcloud#7201. Features: • Smart diff rendering with emoji indicators (➕✏️🗑️🚚) • Word-level diffing for modifications with <del>/<ins> HTML tags • Intelligent move detection for relocated unchanged content • Special handling for checkbox state transitions (☐ ↔ ☑) • Line number context preservation for better readability • Noise filtering (empty lines, unchanged content) Technical Implementation: • New DiffService with LCS-based diff algorithm • Enhanced activity provider with diff integration • Multi-pass detection: moves → modifications → additions/deletions • Similarity scoring system for optimal operation pairing • HTML-safe output compatible with activity feed constraints Benefits: • Clear visualization of content changes in activity timeline • Reduced cognitive load with filtered, relevant changes only • Professional emoji-based change indicators • Maintains backward compatibility with existing activity system The system intelligently distinguishes between: - Line moves (🚚): Exact content relocated to different position - Modifications (✏️): Content changes with word-level highlighting - Additions (➕): New content inserted - Deletions (🗑️): Content removed Tested extensively with various scenarios including complex multi-operation changes, edge cases, and performance considerations. Signed-off-by: Alexander Askin <lexioj@gmail.com>
- Apply PHP-CS-Fixer formatting to DiffService.php - Fix tab indentation and control structure braces - Align with Nextcloud coding standards - Update test expectation for visual diff feature - Adjust DeckProviderTest to expect new diff output format - Test now expects '✏️1 <del>ABC</del>→<ins>BCD</ins>' instead of 'BCD' Fixes failing php-cs and phpunit checks in CI Signed-off-by: Alexander Askin <lexioj@gmail.com>
- Fix line numbering to consistently show positions in NEW text - Use dependency injection for DiffService in DeckProvider - Update copyright year to 2025 - Extract checkbox pattern as class constant - Remove unused $html parameter and $removeIndex variable - Fix duplicate remove operations tracking - Enhance checkbox change detection to handle text modifications - Add special line formatting for code blocks, callouts, and quotes - Improve visual presentation with emojis and separators - Shorten move notation from "(moved from line X)" to "(from X)" Signed-off-by: Alexander Askin <lexioj@gmail.com>
- Remove HTML tags (<ins>, <del>) from diff output for activity feed compatibility - Add callout block emoji transitions (e.g., ℹ️→🔴 for ::: info → ::: error) - Use plain text with arrows (→) to show changes instead of HTML formatting The activity feed doesn't support HTML tags properly, causing them to appear as escaped text. This change uses plain text formatting that displays correctly. Signed-off-by: Alexander Askin <lexioj@gmail.com>
- Add quote line detection and formatting for MODIFY operations to prevent > display issues - Combine consecutive <ins> and <del> tags to reduce visual clutter - Merge whitespace-only keep operations between add/remove tags for cleaner output - Update emoji symbols for better visual representation: - Error callout: 🔴 → ❗ (exclamation mark) - Quote blocks: 💬 → ❞ (quotation mark) - Code blocks: 📝 → ‹› (guillemets) Signed-off-by: Alexander Askin <lexioj@gmail.com>
LexioJ
force-pushed
the
feature/visual-diff-7201-v1.15.2
branch
from
4381b10 to
a03dfdc
Compare
- Move $usedRemoves assignment outside if-else block to reduce duplication - Merge remove/keep switch cases in renderWordLevelHtml as they have identical logic Addresses code review comments from @luka-nextcloud
|
@nickvergessen would there be a way to achieve something like this or to add support for it? |
|
It would need implementing/support in all mobile apps and desktop clients that show activity entries. Could either use Or you add a new one, which has only the new text as name (which is shown when clients don't know the type) and has a diff parameter using ins and del. Then here in the deck app rendering we can show fancy html, and in generic clients we save the space and only show the new description? |
|
Sounds like a good compromise to me! Or all other places could actually just link to the deck activity tab without detailing the exact changes. |
I created a screenshot with annotations to illustrate what this code aims to achieve - if there are better ideas or suggestions, would be happy to read them 😀 - overall goal is to make it easier to understand what actually changed on a given modification within the description field.
|
|
In all honesty, not sure this is only me, but as a heavy deck user apart from:
And maybe but less sure on this one:
I don't find any of the other things useful and it just adds noise to the activity stream. Maybe this could therefore be limited to the in deck card panel view, but render with the subject and no message? |
|
A few comments from my side
@juliusknorr do you have some thoughts here? |
Proposed Improvements Based on FeedbackThank you @marcoambrosini, @nickvergessen, and @luka-nextcloud for the detailed feedback. I've heard your concerns about noise and complexity. Here's a revised approach that addresses these issues: Key Changes
Visual ExamplesCheckbox Changes (Simple & Clear)Before: After: Text Modifications (Focused on What Changed)Before: After: Small Additions/DeletionsBefore: After: Or for deletions: Combined Changes (Multiple Operations)Before: After: Large Changes (Noise Reduction)When someone rewrites substantial portions (10+ lines changed): Instead of showing every detail: This prevents the activity feed from being overwhelmed while still indicating significant work happened. Callout Block Changes (Simplified)Before: After: Or simply include it in the text diff: Implementation StrategyThe core algorithm remains the same (LCS-based diff detection), but the presentation layer becomes much cleaner:
Addressing Specific Concerns@nickvergessen's concern about noise:
@marcoambrosini's preferences:
What Stays HiddenTo keep activity clean, these won't generate entries:
Open Questions
I believe this approach balances the need for clarity (which my users are requesting) with the maintainers' valid concerns about noise. The activity feed stays clean for typical use, while still providing actionable information when descriptions change. Happy to implement these changes if this direction looks good to you! |
🤷 HTML is not allowed in descriptions and messages of activity
Same here, activity entries don't have any logic. The entries are shown in iOS, Android and Desktop apps. We can not add JS/HTML code, as they are shown unrendered |
To clarify here @nickvergessen, does this limitation apply for the activity app only, or also for the activity tab of the deck card? |
It's using the same API |
5 participants
🎯 Overview
This PR introduces a comprehensive visual diff system that transforms how card description changes are displayed in the activity feed, making it easier for users to understand what exactly changed in their content.
Resolves #7201
✨ Features
🔍 Smart Change Detection
<del>/<ins>HTML tags for cleaner output>display issues)🎭 Professional Visual Indicators
Old contentNew content"🧠 Intelligent Filtering & Optimization
<ins>and<del>tags to reduce visual clutter<del>text 1 text 2</del>instead of<del>text 1</del> <del>text 2</del>)🛠 Technical Implementation
Core Components
DiffService: Service implementing LCS-based diff algorithm with multi-pass detectionDeckProvider: Updated activity provider with dependency injection for DiffService>issues)Algorithm Highlights
<ins>/<del>tags and whitespace-only keep operationsActivity Feed Compatibility Fixes
>character to prevent>display issues📸 Visual Examples
Word-Level Changes
Before:
John Doe changed the descriptionAfter:
✏️2 Update <del>deadline</del><ins>timeline</ins>Checkbox Changes
Before:
John Doe changed the descriptionAfter:
✏️1 🔲→☑️ Task completedCallout Block Changes
Before:
John Doe changed the descriptionAfter:
✏️5 ℹ️→❗Quote Changes
Before:
John Doe changed the description(displayed as> text)After:
✏️3 →❞ Important noteCombined Tags (Cleaner Output)
Before:
✏️5 <del>```</del><ins>-</ins><ins> </ins><ins>[x]</ins><ins> </ins><ins>ToDo</ins><ins> </ins><ins>1</ins>After:
✏️5 <del>```</del><ins>- [x] ToDo 1</ins>🧪 Testing
Extensively tested with various scenarios:
>fix🚀 Benefits
For Users
>characters in activity feedFor Developers
🔧 Configuration
No configuration required - the feature works out of the box with existing Deck installations.
📋 Changelog
Latest Updates
>display issues in activity feed for quote lines<ins>/<del>tagsCode Review Feedback Addressed
🤝 Contributing
This implementation focuses on card description changes but the
DiffServicearchitecture is designed to be extensible for other content types in the future.This PR represents a significant enhancement to user experience by providing clear, actionable insights into content changes while maintaining the professional quality expected from Nextcloud applications.