Closed sevimuelli closed 5 months ago
Many thanks, this looks reasonable.
Could you show some screenshots form before your changes? I was actually wondering how to enable all those buttons. I can add one to create still images (when enabled), but the others are, I guess, added depending on the camera device, whether it supports zooming, moving, a light, or other functions?
Previously only the camera name and motionEye buttons are at the top, the framerate at the button left and the action buttons at the bottom right. It makes sense that they can exceed the camera frame, or do some ugly line break when being too many.
I wonder whether it makes sense to leave the camera name larger in one row, and the framerate and optional monitoring info in two rows, like it was before? For identifying reasons, the camera name might better large. Or alternatively, probably the best solution, have the framerate below the camera name, as it is always available, and the monitoring info as optional second column. That way also on very small camera frames, if no monitoring info is shown, there is more space for all info at the top. It is a different topic, but in my case, and I see the same in one of your cases, the monitoring info is an empty string, and shown as ""
, instead of just hidden. We should just set it empty, to have it omitted completely, in this case.
Side note: The CodeFactor annotations are unrelated to this PR and, AFAIK, wrong, since those functions/variables are in fact used from HTML elements.
I tested the PR as patch, and it works as intended. One needs to do a forced-reload to load the new frontend code. We really need to add some versioning for this, but a different topic.
This is how it looked before: To enable the buttons one simply needs to add the corresponding files under /etc/motioneye. For example up_1, down_1, preset1_1 and so forth. For testing, they don't even need to do something. An empty file that is executable is enough to show up in motioneye.
I think your idea with the optional second column is the best idea. I will implement that. I noticed that on small screens, it is very crowded. i fixed the issue with the monitoring info. The problem was that even if there is no info, the string contains "". So the value was "\"\"". Now this case will be ignored.
Considering the CodeFactor, this is actually correct. The four values showEmbedUrl, authorizeUpload, showMjpgUrl, showSnapshotUrl can't be found because as you said, they are used in html. There I added the comment "// eslint-disable-next-line no-unused-vars" so they don't throw an error anymore. But the other two, layoutRows and remCameraFrameUi are indeed not used. They were used before but the code changed and it was not removed. I commented those out with a remark and will clean it up properly in a next pull request.
I moved the monitoring info to a second column which only appears if needed.
Is this how you imagined it or would you like me to change something?
I moved the monitoring info to a second column which only appears if needed.
I indeed meant a "column", not a "row" what you added now. So basically switching framerate and monitoring info positions form how the PR was at first, and then have this 2nd column removed if no monitoring info is available.
But actually I like the solution with a 3rd row more, respectively a 2nd row considering how elements are nested. Allow text information listed below each other IMO is clearer, and I think this little additionally removed/overlaid camera view is worth it. So from my end: Let's keep what you have now 👍.
Thanks for addressing the CodeFactor annotations. Exactly how I'd have done in a dedicated PR. About the layout rows: I remember we recognised that there is not really a point to allow defining layout rows and columns both, and that even changing the amount of rows did not have any effect on tests: It is sufficient to just allow defining the amount of columns (camera frames per line) and the amount of rows is implicitly defined by the amount of cameras instead. I thought we did some change in this regards already, but could not find the PR. So I guess this part was broken in 0.42.1 already.
Currently, the GUI settings still allow to define the amount of rows. Do you mean, while this is possible, and indeed changes the layoutRows
variable, it does not have an effect in JS, as the variable is never called for arranging the camera frames? Then we should just remove the setting as well. Against something we can do in a separate PR. Did you find out what the original purpose of remCameraFrameUi
was? I just want to avoid that we remove traces of something which was intended to work and unintentionally broken. But if it is intentionally not used anymore, we of course can remove the commented definition as well.
EDIT: Ah, found it in the commit you linked. So it was to hide the camera frames, which is now done differently. So can be definitely removed (the comment).
So from my standpoint I think everything is completed so far. If you have any other ideas / improvements I'm happy to implement it.
Concerning you points about the CodeFactor issues, thank you for clarifying this. I would like to do this properly to check for any leftovers, not just delete the blocks I commented out. As soon as this PR is merged, I will do a proper cleanup and open a new PR. I really want to check where all those variables are used and what can be removed without removing too much. 😅
Great work!
Let's do the cleanup right away. We have this PR as reference and the commits you referenced for the obsolete variables/functions remain visible here.
I'll do a final test this weekend and merge it if everything works well.
I moved the calculation for the action buttons to a separate callback function which only gets executed when the layout updates. Before it was executed once a second which is quite unnecessary. But now really everything is done, so you can test it. Thank you.
Hi, could you do the testing and merge this weekend, so I could to the cleanup in the next step as we talked about?
Hi
I made some UI changes to increase the available action buttons. There have been several requests concerning this. This pull request fixes: #613 ,#844, #1766.
I moved the frame rate and monitoring info to the top. I tried to optimize everything for space constraints. There are two rows in the top left. First one for camera name and frame rate and the second one for monitoring info. If there is no monitoring info, the second row disappears and name and fps are centered. The bottom is now solely for action buttons. Instead of 8, it allows up to 16. If there are only a few or if the window is wide enough, there is only one row. If more are added or the view changes and the camera window gets smaller, a second row appears.
There are no breaking changes or anything, simply a UI enhancement. The only thing I am not super happy with for now is that in order to be able to make the buttons evenly spaced ( I used flex), I need to check the width of the camera window and make the settings in css accordingly. To do so, I check on a regular basis for the width. The only place I have found so far is in the main.js where I found a function that get's executed once a second to update frame rate and stuff. This is the main code for on line 5096 in main.js:
But I don't need to check it regularly, only when the UI changes. There is this function "updateLayout();" in main.js, which gets called only when the view changes but I don't see a way to access the individual data of each camera-window inside this function. Any recommendations are welcome. But I think for now, this isn't to big of a problem.