mraible commented on code in PR #233: URL: https://github.com/apache/roller/pull/233#discussion_r4458011210
########## app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/MediaFileDisplay.java: ########## @@ -0,0 +1,118 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. The ASF licenses this file to You + * under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. For additional information regarding + * copyright in this work, please see the NOTICE file in the top level + * directory of this distribution. + */ + +package org.apache.roller.weblogger.ui.struts2.editor; + +import java.util.Locale; + +import org.apache.commons.lang3.StringUtils; + +/** + * Short, human-readable labels for media files, shared by the media library + * and the editor's media chooser. + */ +public final class MediaFileDisplay { + + private static final String[] UNITS = {"KB", "MB", "GB", "TB"}; + + private MediaFileDisplay() { + } + + /** + * Size for display: "512 bytes", "1.5 KB", "23 MB". One decimal below 10 + * of a unit, none above. + */ + public static String formatSize(long bytes) { + if (bytes < 1024) { + return bytes == 1 ? "1 byte" : bytes + " bytes"; + } + double value = bytes; + int unit = -1; + do { + value /= 1024; + unit++; + } while (value >= 1024 && unit < UNITS.length - 1); Review Comment: Non-blocking: the unit is picked before rounding, so 1048575 bytes shows as "1024 KB" instead of "1 MB" (and 1073741823 as "1024 MB"). Continuing the loop while `value >= 1023.5`, the point where `%.0f` rounds up to 1024, would fix it. A test case for 1048575 bytes would catch it. ########## app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/MediaFileEdit.java: ########## @@ -181,6 +216,42 @@ 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. getLibraryURL() sets + * the weblog itself, so the link always points at this weblog's library. + */ + @StrutsParameter + public void setReturnQuery(String returnQuery) { + this.returnQuery = returnQuery != null && returnQuery.length() <= 2000 + && returnQuery.matches("[A-Za-z0-9._~%&=+-]*") ? returnQuery : null; Review Comment: Non-blocking: `URLEncoder` leaves `*` unencoded, so a library search for `IMG*` produces a returnQuery of `bean.name=IMG*`. This regex rejects it, and Back to library drops the search, sort and page. Adding `*` to the class (`[A-Za-z0-9._~%&=+*-]*`) covers everything `getLibraryQuery()` can produce. ########## app/src/main/java/org/apache/roller/weblogger/ui/struts2/editor/MediaFileAdd.java: ########## @@ -191,50 +218,108 @@ public String save() { if (!WebloggerFactory.getWeblogger().getFileContentManager().canSave( getActionWeblog(), fileName, declaredType, uploadFile.length(), errors)) { + failed(fileName, messageText(errors)); continue; } mediaFile.setContentType(MediaTypePolicy.storedTypeFor( mediaFile.getName(), declaredType, RollerContext.getServletContext()::getMimeType)); manager.createMediaFile(getActionWeblog(), mediaFile, errors); + if (errors.getErrorCount() > 0) { + failed(fileName, messageText(errors)); + continue; + } WebloggerFactory.getWeblogger().flush(); if (mediaFile.isImageFile()) { newImages.add(mediaFile); } else { newFiles.add(mediaFile); } - - uploaded.add(mediaFile); + uploadResults.add(UploadResult.uploaded(mediaFile)); } catch (Exception e) { - log.error("Error uploading media file", e); - addError("mediaFileAdd.errorUploading", bean.getName()); + log.error("Error uploading media file " + fileName, e); + failed(fileName, getText("mediaFileAdd.errorUploading", + Collections.singletonList(fileName))); } } - for (Iterator<RollerMessage> it = errors.getErrors(); it.hasNext();) { - RollerMessage msg = it.next(); - addError(msg.getKey(), Arrays.asList(msg.getArgs())); + if (newImages.isEmpty() && newFiles.isEmpty()) { + // nothing was saved; the errors are on the upload form + return result(INPUT); } - if (!uploaded.isEmpty() && !this.errorsExist()) { - addMessage("uploadFiles.uploadedFiles"); - for (MediaFile upload : uploaded) { - addMessage("uploadFiles.uploadedFile", - upload.getPermalink()); - } + // Files that were saved are listed on the success page, with + // any that failed, so a partial upload is never hidden. + this.pageTitle = "mediaFileAddSuccess.title"; Review Comment: Non-blocking: when a non-JS upload partly succeeds, each failed file shows twice. It appears once in the error box from messages.jsp (via `failed()`'s `addActionError`) and again in the success page's failed list. To reproduce, upload `a.png` plus a disallowed `b.exe` with JavaScript off. Calling `clearActionErrors()` before returning SUCCESS here, or only adding the action error when the result is INPUT, would list it once. -- 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]
