-
Notifications
You must be signed in to change notification settings - Fork 300
🎨 Enhanced Visual Diff System for Card Description Changes (Resolves #7201) #7212
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
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. |
b5f9e4b
to
86df615
Compare
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.) |
2a58b7c
to
40315b9
Compare
40315b9
to
0f5ec86
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lib/Activity/DeckProvider.php
Outdated
$this->l10nFactory = $l10n; | ||
$this->config = $config; | ||
$this->cardService = $cardService; | ||
$this->diffService = new DiffService(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The DiffService can be inject via the constructor instead of instantiating it directly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Year in copyright text should be 2025
lib/Service/DiffService.php
Outdated
*/ | ||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The checkbox pattern can be defined as a constant to avoid duplicate code
lib/Service/DiffService.php
Outdated
* @return string | ||
*/ | ||
private function generateCheckboxDiff(string $oldLine, string $newLine): string { | ||
$checkboxPattern = '/^(\s*-\s*)\[([ xX])\](.*)/i'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The checkbox pattern can be defined as a constant to avoid duplicate code
lib/Service/DiffService.php
Outdated
* @param array $newLines | ||
* @return string | ||
*/ | ||
private function enhanceWithWordLevelDiff(string $html, array $operations, array $oldLines, array $newLines): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The $removeIndex is not used
5cca5d9
to
bdd5fe0
Compare
6c5db9d
to
60eb522
Compare
314af8a
to
57c59a5
Compare
57c59a5
to
c9f2fc0
Compare
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 <[email protected]>
- 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 <[email protected]>
- 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 <[email protected]>
- 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 <[email protected]>
- 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 <[email protected]>
c9f2fc0
to
c1c3510
Compare
🎯 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 description
After:
✏️2 Update <del>deadline</del><ins>timeline</ins>
Checkbox Changes
Before:
John Doe changed the description
After:
✏️1 🔲→☑️ Task completed
Callout Block Changes
Before:
John Doe changed the description
After:
✏️5 ℹ️→❗
Quote Changes
Before:
John Doe changed the description
(displayed as> text
)After:
✏️3 →❞ Important note
Combined 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
DiffService
architecture 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.