Fit vertically 2x2 as expected #2590
Conversation
|
Many thanks. Please try to avoid trailing spaces, indentation changes and code style changes, to keep the code changes better reviewable. I'll do some tests when I find time. |
There was a problem hiding this comment.
Looks sensible and likely working solution to me. The only things I'm somewhat wondering (and which could be quite easily tested, I admit) are:
- How does it work when rows x cols is less than the number of configured cameras? Reshaping the camera array in that case seems to drop off the extra, will that happen to camera frames, too?
- How does it work when there are more than four rows (or four columns for that matter, I see you took inspiration from the original column layout)? The number of "row" (and "column") CSS classes can be increased, but given the dynamic nature of the layout, should we instead have the height (and width) set with inline CSS so the code would work regardless of the setup size? Or is this a non-issue since the number of rows and cols is limited by the sliders in the UI? In that case it's all fine 😃
- How does it work when some cameras are in portrait orientation and some in landscape? Might not be very usual but some users have that kind of setup.
Please fix the indentations that were broken toward the end of main.js.
|
Yeah, my editor wanted to make use of indents/spaces consistent throughout this file so it changed the unrelated lines. I can override that so it's just changing lines relevant to this issue I agree, a mix of landscape/portrait cameras complicates things. Maybe let's tackle that later (right now, I think releasing this would be better than the current state of affairs even if not covering 100% of use cases - we can follow up if users complain). Since CSS prevents having individual frames be different, follow ups would probably be: 1) making sure "combinedRatio" is OK as-is (which I think it is) and 2) apply a CSS class to each camera that inherits frame width or frame height, whichever makes sure that the cam fit into its frame regardless of its own aspect ratio >1 or <1 and whether its frame's aspect ratio >1 or <1 The UI probably shouldn't let you choose rows/columns that are inconsistent with the # of cameras chosen. Might have to fire an event when the user adds or deletes a camera, so that they adjust the rows/columns right then. There's no sensible default handling if you change # of cameras (you don't want to hide cameras, you don't want extra blank rows/columns with no cams) |
|
Those points I raised were only things that popped up in my mind while reviewing the code, they might well be non-issues. At least what you wrote on them makes sense to me. I agree that if by testing we can confirm that this PR generally improves things, even if leaving some earlier not-that-well handled edge cases dealt in a non-optimal way, it would make sense to merge. I don't even have any idea how to properly deal with great variations in the frame aspect ratios, other than maybe ordering them automatically so that similar frames are on the same row or column. But the users might anyway like to order the cameras themselves in their liking (and I think there's at least one issue for being able to change the order of the cams), so maybe there's not much use in trying to do anything about it here. |
|
Pushed a new commit that reverts the unrelated whitespace change as discussed Also noticed a regression relating to single-cam view not rendering correctly, so added a fix for it. @MichaIng does this look OK? |
Closes #2583