snoopdave commented on code in PR #233:
URL: https://github.com/apache/roller/pull/233#discussion_r4438093585
##########
app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/MediaFileEdit.java:
##########
@@ -181,6 +210,37 @@ public String save() {
return INPUT;
}
+ public MediaFile getMediaFile() {
+ return mediaFile;
+ }
+
+ public String getReturnQuery() {
+ return returnQuery;
+ }
+
+ /**
+ * The library page to return to. Only the query string comes from the
+ * request, and only URL-safe characters are kept, so the link always
+ * points at the library of this weblog.
Review Comment:
🐞Claude Issue: **Minor:** this comment says the link "always points at the
library of this weblog", but `returnQuery` can carry a different `weblog=`
value, so the link can go to another weblog's library. That page checks
permissions, so this is about accuracy only. Either correct the comment, or
have `getLibraryURL()` set the `weblog` parameter itself and drop any `weblog`
from `returnQuery`.
##########
app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/MediaFileSearchBean.java:
##########
@@ -173,37 +245,34 @@ public void copyTo(MediaFileFilter dataHolder) {
}
dataHolder.setSizeFilterType(sftype);
- long filterSize = this.size ;
+ long filterSize = this.size;
if ("mediaFileView.kb".equals(this.sizeUnit)) {
filterSize = this.size * RollerConstants.ONE_KB_IN_BYTES;
} else if ("mediaFileView.mb".equals(this.sizeUnit)) {
- filterSize = this.size * RollerConstants.ONE_MB_IN_BYTES;
+ filterSize = this.size * RollerConstants.ONE_MB_IN_BYTES;
}
dataHolder.setSize(filterSize);
}
- if (!StringUtils.isEmpty(this.tags)) {
- dataHolder.setTags(Arrays.asList(this.tags.split(" ")));
+ if (StringUtils.isNotBlank(this.tags)) {
+
dataHolder.setTags(Arrays.asList(StringUtils.split(this.tags.trim())));
}
- dataHolder.setStartIndex(pageNum * PAGE_SIZE);
-
+ dataHolder.setStartIndex(page * PAGE_SIZE);
Review Comment:
🐞Claude Issue: **Important:** `page * PAGE_SIZE` is computed as an `int`. A
very large `bean.page` (for example `50000000`) overflows to a negative start
index. `JPAMediaFileManagerImpl.searchMediaFiles` treats a negative index as
"no paging", so the page loads every file in the folder. The past-the-end check
in `MediaFileView.execute()` does not catch a negative index.
Fix: cap the page in `setPage`, for example `this.page =
Math.min(Math.max(0, page), Integer.MAX_VALUE / PAGE_SIZE);`, or compute the
index as a `long` and clamp it. The existing past-the-end check then moves the
page back to the last one.
##########
app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/MediaFileImageChooser.java:
##########
@@ -76,55 +86,73 @@ public List<String> requiredWeblogPermissionActions() {
*
* @return String The result of the action.
*/
+ /**
Review Comment:
🐞Claude Issue: **Minor:** `execute()` now has two Javadoc blocks. The old
one above this ("Fetches and displays list of media file...") is no longer
accurate. Remove it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
