feature for adding multiple magnet links#1313
feature for adding multiple magnet links#1313jure965 wants to merge 6 commits intotransmission-remote-gui:masterfrom jure965:master
Conversation
PeterDaveHello
force-pushed
the
master
branch
3 times, most recently
from
edddb71 to
be92509
Compare
![cr-gpt[bot]](https://avatars.githubusercontent.com/in/292659?s=60&v=4){kind=small}
| WordWrap = False | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Based on the code patch provided, the following observations can be made:
- The form's size and position have been changed.
- The border style has been removed from the form.
- The
Buttonscomponent's top property has been modified to adjust its position based on the new height of the form. - A new
meLinkcomponent (TMemo) has been added to replace the oldedLinkcomponent (TEdit). The new component allows for multiple lines of input and has a vertical scrollbar that appears when needed. - The caption of the
txLinklabel has been updated to reflect that multiple links can be entered.
As far as potential improvements go, this code patch seems to be relatively minor and doesn't introduce any obvious bugs. One suggestion could be to validate input in the meLink component to ensure that only valid .torrent files or magnet links are accepted. Another suggestion could be to add tooltips to help users differentiate between entering a single link versus entering multiple links.
PeterDaveHello
left a comment
PeterDaveHello
left a comment
There was a problem hiding this comment.
Some not irrelevant changes should be removed, thanks.
| <DestinationDirectory Value="\home\pi\project\0"/> | ||
| <IgnoreBinaries Value="False"/> | ||
| <IncludeFileFilter Value="*.(pas|pp|inc|lfm|lpr|lrs|lpi|lpk|sh|xml)"/> | ||
| <ExcludeFileFilter Value="*.(bak|ppu|ppw|o|so);*~;backup"/> |
There was a problem hiding this comment.
Is this related to the topic? I can't tell.
There was a problem hiding this comment.
will undo changes on this file, they were probably done by my IDE
| WordWrap = False | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
The code patch seems to include changes to the visual layout of a form.
Some improvements could be made to improve the user experience, such as labeling the "Add" button more clearly and adding validation to ensure that the user enters a valid URL or magnet link.
There don't appear to be any immediate bug risks in the code. However, it's difficult to assess the quality of the code as a whole without seeing the complete program.
|
Summary By MatterAI
🔄 What ChangedUpdated project configuration in 🔍 Impact of the Change
📁 Total Files Changed
🧪 Test Added/RecommendedRecommended
🔒 Security VulnerabilitiesNo security vulnerabilities introduced. Configuration-only change with no impact on authentication, input validation, or data handling. ⏳ Estimated code review effortLOW (~7 minutes) Tip Quality Recommendations
♫ Tanka Poem
Sequence DiagramsequenceDiagram
participant IDE as Lazarus IDE
participant Build as Build System
participant Deploy as Deployment Pipeline
IDE->>Build: Load transgui.lpi
Note over IDE,Build: Project config with Version=10, RevisionNr=0
Build->>Build: Apply IncludeFileFilter
Build->>Build: Exclude bak|ppu|o|so|~|backup
Build->>Deploy: Publish to \home\pi\project\0
Deploy-->>Build: Confirmation
IDE->>IDE: Run via xterm -T 'Lazarus Run Output'...
Note over IDE: Using updated RunParams format
|
WalkthroughMulti-line URL input replaces single-line entry; AddTorrent dialog gains an "Apply to all" checkbox; Main form adds FApplyAll and a MultipleTorrents parameter. Various UI layouts were adjusted across forms; dialog control flow was updated to support batch add operations and dialog skipping. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant MainForm
participant AddLinkForm
participant AddTorrentForm
User->>MainForm: Click "Add Link"
MainForm->>AddLinkForm: Show dialog
Note right of AddLinkForm: multi-line memo (one URL per line)
User->>AddLinkForm: Enter URLs and click OK
AddLinkForm->>AddLinkForm: Iterate lines, trim, remove empties
alt At least one URL
AddLinkForm->>MainForm: Return URL list
MainForm->>MainForm: Set MultipleTorrents flag if >1
loop For each URL
MainForm->>AddTorrentForm: Show (cbApplyAll visible if MultipleTorrents)
alt ApplyToAll set
MainForm->>MainForm: Skip dialog for remaining URLs
else
User->>AddTorrentForm: Configure & confirm
AddTorrentForm->>MainForm: Optionally set ApplyToAll
end
MainForm->>MainForm: Process torrent
end
else No valid URLs
AddLinkForm->>AddLinkForm: Show error, focus memo
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
addlink.pas (1)
68-87: Multi-line processing logic is sound, but consider validation enhancements.The implementation correctly:
- Iterates backwards to safely delete empty lines during iteration
- Trims whitespace from each line
- Validates that at least one non-empty link exists
However, consider these enhancements:
- URL validation: Add basic validation to verify each line is a valid URL or magnet link format before accepting
- Error message clarity: The error message "No link was specified" uses singular form, but the UI now accepts multiple links. Consider updating to "No links were specified" or "At least one link must be specified"
Example validation you could add:
for i := meLink.Lines.Count - 1 downto 0 do begin meLink.Lines[i]:=Trim(meLink.Lines[i]); - if meLink.Lines[i] <> '' then ok := true // have at least one link + if meLink.Lines[i] <> '' then + begin + // Basic validation: check if it looks like a URL or magnet link + if (Pos('http://', LowerCase(meLink.Lines[i])) = 1) or + (Pos('https://', LowerCase(meLink.Lines[i])) = 1) or + (Pos('magnet:?', LowerCase(meLink.Lines[i])) = 1) then + ok := true + else + meLink.Lines.Delete(i); // Remove invalid lines + end else meLink.Lines.Delete(i); // remove empty lines end;addlink.lfm (1)
51-51: Minor: Consider adding a space in the label caption.The caption reads
'URL of a .torrent file or a magnet link(one per line):'with no space before the parenthetical. Consider:'URL of a .torrent file or a magnet link (one per line):'- Caption = 'URL of a .torrent file or a magnet link(one per line):' + Caption = 'URL of a .torrent file or a magnet link (one per line):'main.pas (4)
736-736: Avoid repeating default parameter in the implementation header.Some Lazarus/FPC versions warn/error if a default value is repeated outside the declaration. Keep the default only in the class declaration and drop it from the implementation header.
Apply:
- function TMainForm.DoAddTorrent(const FileName: Utf8String; const MultipleTorrents: boolean = false): boolean; + function TMainForm.DoAddTorrent(const FileName: Utf8String; const MultipleTorrents: boolean): boolean;Please confirm it builds across your supported Lazarus/FPC versions.
Also applies to: 2329-2329
2307-2323: Harden multi-link input (trim, skip blanks, dedupe).Current loop may attempt to add empty/whitespace lines. Also, deduping avoids duplicate adds. Minimal change below:
- begin - FApplyAll:=false; - for i := 0 to meLink.Lines.Count - 1 do - begin - if isHash(meLink.Lines[i]) then meLink.Lines[i] := 'magnet:?xt=urn:btih:' + meLink.Lines[i]; - DoAddTorrent(meLink.Lines[i], meLink.Lines.Count > 1); - end; - FApplyAll:=false; - end; + begin + FApplyAll := false; + // Work on a copy to allow trimming/dedup without mutating the control + with TStringList.Create do + try + Assign(meLink.Lines); + // normalize + for i := 0 to Count - 1 do begin + Strings[i] := Trim(Strings[i]); + if isHash(Strings[i]) then + Strings[i] := 'magnet:?xt=urn:btih:' + Strings[i]; + end; + // remove empties and duplicates + BeginUpdate; + for i := Count - 1 downto 0 do + if Strings[i] = '' then Delete(i); + Sorted := True; Duplicates := dupIgnore; // then restore original order isn’t required here + Sorted := False; + EndUpdate; + + for i := 0 to Count - 1 do + DoAddTorrent(Strings[i], Count > 1); + finally + Free; + end; + FApplyAll := false; + end;
2750-2756: Apply-to-all UX scope and consistency.
- ok considers FApplyAll (good) and hides the dialog for subsequent items.
- cbApplyAll.Visible toggles only when MultipleTorrents=True, so the checkbox is unavailable in other batch sources.
Suggestion: propagate MultipleTorrents to all batch entry points (e.g., file/clipboard watcher) so the same “Apply to all” UX is available there. Also, consider whether selecting all files (btSelectAllClick) for subsequent items matches user intent; if not, persist more of the first dialog’s choices.
Also applies to: 2846-2848
7875-7883: Expose “Apply to all” for queued additions as well.CheckAddTorrents calls DoAddTorrent without the MultipleTorrents flag, so users won’t see the checkbox when multiple items are pending (watch folder, IPC, etc.). Pass a precomputed flag:
- FApplyAll:=false; - while FPendingTorrents.Count > 0 do begin + FApplyAll := false; + // Show "Apply to all" when there is more than one pending item + // Compute once so it stays true across the loop. + // (If you prefer dynamic, recompute inside the loop.) + var multi := FPendingTorrents.Count > 1; + while FPendingTorrents.Count > 0 do begin s:=FPendingTorrents[0]; FPendingTorrents.Delete(0); if s <> '' then - DoAddTorrent(s); + DoAddTorrent(s, multi); end; - FApplyAll:=false; + FApplyAll := false;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
addlink.lfm(2 hunks)addlink.pas(3 hunks)addtorrent.lfm(6 hunks)addtorrent.pas(2 hunks)main.lfm(27 hunks)main.pas(6 hunks)transgui.lpi(2 hunks)
👮 Files not reviewed due to content moderation or server errors (1)
- main.lfm
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: MatterAI Code Review
🔇 Additional comments (9)
addtorrent.pas (2)
55-55: LGTM: Checkbox control declaration looks correct.The
cbApplyAllcheckbox control is properly declared and follows Pascal naming conventions.
99-99: LGTM: Public field for "Apply to All" state.The
ApplyToAllboolean field is appropriately scoped as public for use by the calling code. The naming is clear and follows conventions.transgui.lpi (1)
4-4: Review the version bump.The project version was incremented from 10 to 11. Verify this aligns with your versioning strategy for this feature addition.
addtorrent.lfm (2)
204-211: LGTM: "Apply to all" checkbox properly integrated.The new
cbApplyAllcheckbox is well-positioned and configured with appropriate properties. The caption "Apply to all" clearly indicates its purpose for batch operations.
2-4: Layout adjustments to accommodate new control.The form and control positioning/sizing changes appropriately accommodate the new "Apply to all" checkbox. These appear to be standard layout adjustments.
Also applies to: 78-78, 98-98, 104-104, 108-108, 120-120
addlink.pas (2)
51-51: LGTM: Multi-line input control declared.The change from
edLink: TEdittomeLink: TMemoenables the multi-link feature. This aligns with the PR objectives.
89-93: LGTM: Form initialization properly configured.The
FormCreateprocedure correctly sets up the OK button's modal behavior and click handler.addlink.lfm (1)
2-10: LGTM: UI properly restructured for multi-line input.The form dimensions and control layouts are appropriately adjusted to support multi-line link entry. The
meLinkmemo control is well-configured with:
- Proper anchoring for resize behavior
- WordWrap disabled (appropriate for URL input)
- Adequate height for multiple entries
Also applies to: 16-17, 37-44, 54-62
main.pas (1)
722-722: Good addition: explicit batch-control state.FApplyAll is a simple, readable switch for batch adds and is reset in relevant flows. LGTM.
transgui.lpi
Outdated
| <RunParams> | ||
| <local> | ||
| <FormatVersion Value="1"/> | ||
| <CommandLineParams Value="--home=home"/> | ||
| <LaunchingApplication PathPlusParams="\usr\X11R6\bin\xterm -T 'Lazarus Run Output' -e $(LazarusDir)\tools\runwait.sh $(TargetCmdLine)"/> | ||
| </local> | ||
| <FormatVersion Value="2"/> | ||
| <Modes Count="1"> | ||
| <Mode0 Name="default"> | ||
| <local> | ||
| <CommandLineParams Value="--home=home"/> | ||
| <LaunchingApplication PathPlusParams="\usr\X11R6\bin\xterm -T 'Lazarus Run Output' -e $(LazarusDir)\tools\runwait.sh $(TargetCmdLine)"/> | ||
| </local> | ||
| </Mode0> | ||
| </Modes> |
There was a problem hiding this comment.
🧩 Analysis chain
Address duplicate RunParams structure per previous review feedback.
The RunParams section now contains both the old flat structure (lines 35-38) and the new Modes-based structure (lines 39-47) with FormatVersion="2". This creates redundancy where CommandLineParams and LaunchingApplication are specified twice.
In a previous review, you acknowledged these changes were likely made by your IDE and indicated you would undo them. However, they remain in the current version.
Please verify:
- Whether the old flat structure (lines 35-38) should be removed, or
- Whether this duplication is required by Lazarus IDE for backward compatibility
🏁 Script executed:
#!/bin/bash
# Check if other .lpi files in the repository use similar RunParams structure
fd -e lpi -x cat {} \; | grep -A 10 "RunParams"Length of output: 843
---
🌐 Web query:
```web
Lazarus IDE RunParams FormatVersion 2 structure backward compatibility
💡 Result:
Short answer
- FormatVersion="2" (seen in modern .lpi files) changes RunParams from the older per-launch-section layout (FormatVersion="1", e.g. a node with <LaunchingApplication .../>) to a Modes-based layout (Modes / ActiveMode / Count entries, or a simple with Modes elsewhere). [1][2][3]
- Most recent Lazarus IDE versions understand FormatVersion=2. Very old Lazarus releases (the pre‑1.x era or very early 1.x) expect FormatVersion=1 and may not parse the new Modes structure; in practice that is where backward‑compatibility problems arise. The IDE contains compatibility checks/handling in its compatibility units. [4]
What to do
- If you must keep a project usable in an old Lazarus, either (a) edit the .lpi to the older FormatVersion="1" RunParams structure (restore a section with LaunchingApplication/CommandLineParams), or (b) open/save the project with the older IDE and let it rewrite RunParams. For modern Lazarus just leave FormatVersion="2". [3][1]
Examples / sources
- Example .lpi with RunParams FormatVersion="1" (local + LaunchingApplication). [1]
- Example .lpi snippets using FormatVersion="2" (Modes / simple FormatVersion tag). [2][3]
- Lazarus IDE compatibility unit (handles file/format compatibility rules). [4]
References
[1] Example .lpi (FormatVersion="1" / LaunchingApplication).
[2] Example .lpi (FormatVersion="2", Modes).
[3] More .lpi examples showing FormatVersion="2".
[4] Lazarus IDE compatibility source (compatibilityrestrictions.pas).
Remove old RunParams flat structure; keep FormatVersion="2" Modes-based structure only.
Your file declares FormatVersion="2" which uses the Modes-based layout, but also contains the legacy per-launch-section structure (lines 35-38). These formats are mutually exclusive. Modern Lazarus IDE versions support FormatVersion="2"; remove the old flat <local> section with CommandLineParams and LaunchingApplication at lines 35-38.
🤖 Prompt for AI Agents
In transgui.lpi around lines 34 to 47, remove the old flat <local> section
containing CommandLineParams and LaunchingApplication fields at lines 35 to 38.
Keep only the FormatVersion="2" Modes-based structure starting from line 39,
which includes these settings inside <Mode0>. This will eliminate the
incompatible dual RunParams formats.
|
Closing mainly due to very old pull request. |
2 participants
I never worked with lazarus/pascal before. Please review and test.
Summary by CodeRabbit
New Features
Chores