[vlc-devel] [vlc-commits] macosx: Rework log message storage and fix filtering

[David Fuhrmann]

> Am 07.09.2017 um 03:02 schrieb Marvin Scholz <git at videolan.org>:
> 
> vlc | branch: master | Marvin Scholz <epirat07 at gmail.com> | Thu Sep  7 03:00:15 2017 +0200| [660257430f41f106ecb1834af7ccaf2a37a281de] | committer: Marvin Scholz
> 
> macosx: Rework log message storage and fix filtering
> 
> This is a quite large change to how the log message window data storage
> works. It completely removes the message buffer that was used.
> Instead, the messages are added to the backing array of the array
> controller directly. The array controller will not pick up these changes,
> so adding a lot of messages at once is not a concern.
> The timer is now used to sync the array controller with its backing
> array by calling NSArrayControllers `rearrangeObjects`.
> 
> The array controller is changed to not clear the predicate on adding
> new items, which was the initial reason why the other changes became
> necessary, as the array controller would not allow adding objects
> that did not match the filter predicate. Adding them to the backing
> array and syncing with the array controller by calling rearrangeObjects
> seems to be no problem though.
> 
> This means filtering is now possible while new messages arrive and
> while the filter is set, it will only show new messages matching
> the filter, until the filter is unset, at which point it will show
> all messages again, as expected.
> 
>> http://git.videolan.org/gitweb.cgi/vlc.git/?a=commit;h=660257430f41f106ecb1834af7ccaf2a37a281de
> ---
> 
> modules/gui/macosx/UI/LogMessageWindow.xib  |  2 +-
> modules/gui/macosx/VLCLogWindowController.m | 86 ++++++++++++++---------------
> 2 files changed, 41 insertions(+), 47 deletions(-)
> 
> diff --git a/modules/gui/macosx/UI/LogMessageWindow.xib b/modules/gui/macosx/UI/LogMessageWindow.xib
> index 79d1c01763..e8f3b4422c 100644
> --- a/modules/gui/macosx/UI/LogMessageWindow.xib
> +++ b/modules/gui/macosx/UI/LogMessageWindow.xib
> @@ -442,7 +442,7 @@ IA
>             </toolbar>
>             <point key="canvasLocation" x="139" y="192"/>
>         </window>
> -        <arrayController editable="NO" selectsInsertedObjects="NO" id="0Or-bh-QJU">
> +        <arrayController editable="NO" selectsInsertedObjects="NO" clearsFilterPredicateOnInsertion="NO" id="0Or-bh-QJU">
>             <declaredKeys>
>                 <string>component</string>
>                 <string>message</string>
> diff --git a/modules/gui/macosx/VLCLogWindowController.m b/modules/gui/macosx/VLCLogWindowController.m
> index 22d5c54162..a09839031d 100644
> --- a/modules/gui/macosx/VLCLogWindowController.m
> +++ b/modules/gui/macosx/VLCLogWindowController.m
> @@ -32,16 +32,19 @@
> /* This array stores messages that are managed by the arrayController */
> @property (retain) NSMutableArray *messagesArray;
> 
> -/* This array stores messages before they are added to the messagesArray on refresh */
> - at property (retain) NSMutableArray *messageBuffer;
> -
> /* We do not want to refresh the table for every message, as that would be very frequent when
> - * there are a lot of messages, therefore we use a timer to refresh the table with new data
> - * from the messageBuffer every now and then, which is much more efficient and still fast
> - * enough for a good user experience
> + * there are a lot of messages, therefore we use a timer to refresh the table every now and
> + * then, which is much more efficient and still fast enough for a good user experience.
>  */
> @property (retain) NSTimer        *refreshTimer;
> 
> +/*
> + * Indicates if an update is needed, which is the case when new messages were added
> + * after the last firing of the update timer. It is used to prevent unnecessary
> + * rearranging of the NSArrayController content.
> + */
> + at property (atomic) BOOL           needsUpdate;
> +
> - (void)addMessage:(NSDictionary *)message;
> 
> @end
> @@ -87,8 +90,7 @@ static void MsgCallback(void *data, int type, const vlc_log_t *item, const char
> {
>     self = [super initWithWindowNibName:@"LogMessageWindow"];
>     if (self) {
> -        _messagesArray = [[NSMutableArray alloc] initWithCapacity:500];
> -        _messageBuffer = [[NSMutableArray alloc] initWithCapacity:100];
> +        _messagesArray = [[NSMutableArray alloc] initWithCapacity:1000];
>     }
>     return self;
> }
> @@ -140,7 +142,7 @@ static void MsgCallback(void *data, int type, const vlc_log_t *item, const char
>     vlc_LogSet(getIntf()->obj.libvlc, MsgCallback, (__bridge void*)self);
>     _refreshTimer = [NSTimer scheduledTimerWithTimeInterval:0.3
>                                                      target:self
> -                                                   selector:@selector(appendMessageBuffer)
> +                                                   selector:@selector(updateArrayController:)
>                                                    userInfo:nil
>                                                     repeats:YES];
>     return [super showWindow:sender];
> @@ -152,14 +154,24 @@ static void MsgCallback(void *data, int type, const vlc_log_t *item, const char
>     vlc_LogSet( getIntf()->obj.libvlc, NULL, NULL );
> 
>     // Remove all messages
> -    [self clearMessageBuffer];
> -    [self clearMessageTable];
> +    [self removeAllMessages];
> 
>     // Invalidate timer
>     [_refreshTimer invalidate];
>     _refreshTimer = nil;
> }
> 
> +/**
> + Called by the timer to re-sync the array controller with the backing array
> + */
> +- (void)updateArrayController:(NSTimer *)timer
> +{
> +    if (_needsUpdate)
> +        [_arrayController rearrangeObjects];
> +
> +    _needsUpdate = NO;
> +}
> +
> #pragma mark -
> #pragma mark Delegate methods
> 
> @@ -236,8 +248,7 @@ static void MsgCallback(void *data, int type, const vlc_log_t *item, const char
>     vlc_LogSet(getIntf()->obj.libvlc, NULL, NULL);
> 
>     // Remove all messages
> -    [self clearMessageBuffer];
> -    [self clearMessageTable];
> +    [self removeAllMessages];
> 
>     // Reregister handler, to write new header to log
>     vlc_LogSet(getIntf()->obj.libvlc, MsgCallback, (__bridge void*)self);
> @@ -247,7 +258,7 @@ static void MsgCallback(void *data, int type, const vlc_log_t *item, const char
>  */
> - (IBAction)refreshLog:(id)sender
> {
> -    [self appendMessageBuffer];
> +    [_arrayController rearrangeObjects];
>     [_messageTable scrollToEndOfDocument:self];
> }
> 
> @@ -264,7 +275,7 @@ static void MsgCallback(void *data, int type, const vlc_log_t *item, const char
> 
> /* Called when the user hits CMD + C or copy is clicked in the edit menu
>  */
> -- (void) copy:(id)sender {
> +- (void)copy:(id)sender {
>     NSPasteboard *pasteBoard = [NSPasteboard generalPasteboard];
>     [pasteBoard clearContents];
>     for (NSDictionary *line in [_arrayController selectedObjects]) {
> @@ -301,47 +312,30 @@ static void MsgCallback(void *data, int type, const vlc_log_t *item, const char
> #pragma mark Data handling
> 
> /** 
> - Adds a message to the messageBuffer, it does not has to be called from the main thread, as
> - items are only added to the messageArray on refresh.
> + Adds a message, it is only added visibly to the table on the next firing
> + of the update timer.
>  */
> - (void)addMessage:(NSDictionary *)messageDict
> {
> -    @synchronized (_messageBuffer) {
> -        [_messageBuffer addObject:messageDict];
> -    }
> -}
> +    @synchronized (_messagesArray) {
> +        if ([_messagesArray count] > 1000000) {
> +            [_messagesArray removeObjectsInRange:NSMakeRange(0, 2)];
> +        }
> +        [_messagesArray addObject:messageDict];
> 

Hi Marvin,

Here the array is updated in a possibly separate thread.
Where do you make sure that this lock is also held once the array is read out by the ArrayController in main thread?

BR. David