Event list improvements #963
Conversation
|
💖 Thanks for opening this pull request! 💖 |
|
Build error did not seem to effect the app at all. So it may not matter. Or maybe it's something with my environment setup? C:\Development\zmNinja\platforms\android\app\src\main\res\drawable-land-hdpi\screen.png: Error: The drawable "screen" in drawable-land-hdpi has no declaration in the base drawable folder or in a drawable-densitydpi folder; this can lead to crashes when the drawable is queried in a configuration that does not match this qualifier [MissingDefaultResource] Explanation for issues of type "MissingDefaultResource": As a special case, drawables do not have to be specified in the base There may be scenarios where you have a resource, such as a -fr drawable, (This scenario frequently happens with string translations, where you might 12 errors, 0 warnings |
|
Hmm I thought I was leaving comments in the code. Not sure how they showed up here. Let me try again. |
|
Happy to talk though any of the changes – I was just going to make some small tweaks, but they kept getting bigger and bigger. Sorry, I’ll try and make any future commits more manageable. It may be worth simply compiling the changes and checking out the events list (it’s mostly UI changes despite the large amount of code change), and the new behaviour when you click a notification on the montage view.
|
www/js/NVR.js
Outdated
| if (versionCompare(success.data.version, '1.32.0') != -1) { | ||
| if (versionCompare(success.data.version, '1.34.0') != -1) { | ||
| debug("objdetect supported in image.php"); | ||
| snapshotFrame = 'objdetect'; |
There was a problem hiding this comment.
Should this be controlled by a dial in dev settings? If objdetect shows gif by default it may be quite heavy even if gif animation is being generated.
There was a problem hiding this comment.
Yes, agree. I can add this if you like
There was a problem hiding this comment.
A few things to think about:
a) This will only show gif if gif animation is enabled. If not, it will show the detected object frame - which is lower res than original frames.
b) I noticed elsewhere that when you enable this, I think you are limiting loaded entries to 5? Incase animation is disabled, it may slow UX down
| "-" + nolangTo); | ||
|
|
||
| NVR.getEvents($scope.id, currEventsPage, "", nolangFrom, nolangTo, false, $rootScope.monitorsFilter) | ||
| .then(function (data) { |
There was a problem hiding this comment.
In general a lot of changes made to EventCtrl/events.html. The code here is ugly (i.e. the stuff I wrote) and its been a while since I looked at it. Can you summarize what you did and why?
There was a problem hiding this comment.
When I was changing the height of the image, I found the height of various sections was hardcoded. Most of the changes in the html change the sections to "dynamic" height (fit contents).
www/js/EventCtrl.js
Outdated
| "&height=" + event.Event.thumbHeight * 2; | ||
| snapshotFrame+"&eid="+event.Event.Id + | ||
| "&width=" + event.Event.thumbWidth + | ||
| "&height=" + event.Event.thumbHeight; |
There was a problem hiding this comment.
The reason it was *2 was that if zM scaled it exactly to the resolution, they would show up jaggy on mobile devices
There was a problem hiding this comment.
Hmm - I might have to play with this. I suspect you are right. The previews a do not look "smooth" (i.e. they are jaggy). I guess I thought it was ok (just a preview). But I think I was changing it to support the larger images, not to change the image quality - so I may have this wrong. I'll test this and may add an additional commit to add back the *2.
|
Could you also resolve the small merge conflicts in the next push. Ver has been bumped up to 1.5 in my master. I replaced push plugin with firebasex and cordova versions have been upgraded due ot that. |
Should I merge all changes in the dev branch? |
|
I've already merged dev to master, so continue with master (that is why we got a merge conflict) |
# Conflicts: # package.json # www/js/NVR.js
# Conflicts: # www/js/EventCtrl.js
… events since last notification click
This reverts commit 1400c66.
|
By way of update, when I do the merge I have trouble "compiling". Has this moved from Android SDK 7 to 8? Is the ionic version and other items still the same? I think the problem relates to the new notification plugin - but I can't figure it out. Any tips would be appreciated or at least any indication on build environment changes from the previous version. I'll probably figure it out eventually, but it may take little while (can't afford too many late nights at the moment and still have a day job) :-) |
|
They’ve changed. See updated build from source instructions on versions. |
|
I should have mentioned, I did look at the updated build from source instructions. I was confused as it says to use but the 'ionic info' output shows; |
|
Yup should be 9.0. Did you resolve your build issue? |
|
Yes, looks like I have a working build. In terms of your build instructions these are the steps I followed to get it going. The commands are based on a Windows 10 environment, but I think they are largely transferable to other systems; npm install -g [email protected] @ionic/cli npm i -g cordova-res cd C:\Development\ git clone https://github.com/pliablepixels/zmNinja.git cd zmNinja npm install |
Enables selection of different thumbnail types. If gif is selected limit the number of events returned to 5 to improve load time.
|
Merge conflicts resolved. modified the "enableThumbs" dev option to select different thumb types. Looks to be working fine. Took a bit longer, as I need to upgrade ES to make sure it was all working :) |
… needs cleanup for large thumbs
|
Okay, I have things in a stable state across desktop/iOS/Android. Have tested small/large/none in portrait and landscape mode. Basically, in small I go to a 2 col format, in large a single col format and went back to collection-repeat to get back the performance.
|
|
RE: collection-repeat vs ng-repeat - Now that I've read up on the difference, I see I shouldn't have made this change. I think I made the change to fix the problems I was having with dynamic height of event . Thanks for resolving this. This should also mean that the limit (5) for object_gif is not required. Furthermore, that the code that applies this limit can be removed (this will simplify loadMore() - let me test and make sure it resolves the issue (the view was sluggish with 100 animated gifs - but that was probably ng-repeat and it sounds like collaction-repeat should fix that).
|
|
I think the 5 (or so) for GIF may still be needed. GIF may take up much more resources. |
|
The change to clear the filter when leaving the view, means when you click filter by date/time it does not display the current filter (which it should really). It's also throwing an exception when entering this view when entering the filter screen after clicking the alert in the montage view (which it wasn't doing before). Looking at things now, I think I had changed to ng-repeat as this didn't require a static height to be specified for each item. But, given the performance improvements it seems better to leverage collection-repeat (and thus deal with managing the rowHeight). |
|
I'm pretty happy with how the updates above look with small thumbnails, but I see I have an issue with the large thumbs (at least on Android - images are too wide and disappear of the screen). I'll fix it tomorrow, I need to go to bed - have work tomorrow :) |
#963 show filter text properly, also indicate selective monitors
#963 allow size selection of thumbs
Please make sure you are using the latest master. In my case, manually selecting a filter does show the filter icon
I am not seeing this, at least with master.
Which android OS and device? Not happening on my device
Have you made any changes I should be looking to merge? If so, please make it on master and an updated PR. I don't see any PRs |
#963 show filter text properly, also indicate selective monitors
I believe I merged with the latest master before testing. I will take another look tonight - haven't tried to investigate this yet.
This is in the browser during testing
Yes, is tagging the PR not the right thing to do?. See: |
|
I resolved the Android image size issue I mentioned earlier (problem I introduced with the above commits). I'll look into the filter thing tonight. |
|
You'll have to do a new PR. Tagging doesn't make it pullable. If you are referring to an exception related to |
|
Created a new PR. Just testing now, there seems to be a problem with row height after applying a filter too. I'm happy to dig into both after work... |
|
Ok, let me know what you find in the filter part. I don't see any exception in your situation, but please post the exact results (also testing on a browser). |
|
I've made a commit that addresses the filter concern (I think). The expected behavior: Case 1: Case 2: Note that I am still seeing an occasional exception in event scrolling which is being triggered with the loadMore() code. Need to get into it. |
|
I don't know where I was seeing that exception on the event filter (it's gone now). But the event filter is loading wasn't working for me NVR.debug ("removing montage temporary filter") was being called eveytime. I found a work around for this in my other PR. |
change event list to better take advantage of gifs. Improved event list loading. Click notifications in montage view applies filter so only new events are displayed.