[vlc-devel] [PATCH] macosx: fix startup and shutdown procedure
Rémi Denis-Courmont
remi at remlab.net
Tue Mar 26 17:59:38 CET 2013
Hello,
Felix asked me to review this patch. Unfortunately, I am not intimate to say
the least with the MacOS code in general and its UI in particular. To make
matters worse, I am very upset with accusations against me (<off-topic>of
changing semantics without discussion while I was merely fixing a reoccurrence
of an old bug).
So just to be clear, this review may be inaccurate and read undeservedly rash.
Le mardi 26 mars 2013 15:22:50, Michael Feurstein a écrit :
> * delete playlist root when quitting in order to prevent the playlist from
> playing again during termination
> * removed subscription to LibVLCCore's messages - fixes silent console with
not msg_Dbg coming through
> * added some debug messages during termination to better follow the steps
> * whitespace/trailing space deletion
> * retain on NSArray and NSDictionary - fixes malloc fault during startup
> * screen ID fix - fixes NSScreen fault during startup
That's quite a lot of seemingly unrelated or loosely related stuff in a single
commit.
> ---
> modules/gui/macosx/MainMenu.m | 8 ++++----
> modules/gui/macosx/intf.m | 17 ++++++++++++-----
> modules/gui/macosx/misc.m | 5 ++---
> modules/gui/macosx/playlist.h | 1 +
> modules/gui/macosx/playlist.m | 12 +++++++++---
> src/playlist/thread.c | 2 +-
> 6 files changed, 29 insertions(+), 16 deletions(-)
>
> diff --git a/modules/gui/macosx/MainMenu.m b/modules/gui/macosx/MainMenu.m
> index 0cc28b4..9d05844 100644
> --- a/modules/gui/macosx/MainMenu.m
> +++ b/modules/gui/macosx/MainMenu.m
> @@ -78,7 +78,7 @@ static VLCMainMenu *_o_sharedInstance = nil;
> } else {
> _o_sharedInstance = [super init];
>
> - o_ptc_translation_dict = [NSDictionary
> dictionaryWithObjectsAndKeys: + o_ptc_translation_dict =
> [[NSDictionary dictionaryWithObjectsAndKeys: _NS("Track Number"),
> TRACKNUM_COLUMN,
> _NS("Title"), TITLE_COLUMN,
> _NS("Author"), ARTIST_COLUMN,
> @@ -89,10 +89,10 @@ static VLCMainMenu *_o_sharedInstance = nil;
> _NS("Date"), DATE_COLUMN,
> _NS("Language"), LANGUAGE_COLUMN,
> _NS("URI"), URI_COLUMN,
> - nil];
> + nil] retain];
> // this array also assigns tags (index) to type of menu item
> - o_ptc_menuorder = [NSArray arrayWithObjects:TRACKNUM_COLUMN,
> TITLE_COLUMN, ARTIST_COLUMN, DURATION_COLUMN, -
> GENRE_COLUMN, ALBUM_COLUMN, DESCRIPTION_COLUMN, DATE_COLUMN,
> LANGUAGE_COLUMN, URI_COLUMN, nil]; + o_ptc_menuorder = [[NSArray
> arrayWithObjects:TRACKNUM_COLUMN, TITLE_COLUMN, ARTIST_COLUMN,
> DURATION_COLUMN, + GENRE_COLUMN, ALBUM_COLUMN,
> DESCRIPTION_COLUMN, DATE_COLUMN, LANGUAGE_COLUMN, URI_COLUMN, nil]
> retain]; }
>
> return _o_sharedInstance;
> diff --git a/modules/gui/macosx/intf.m b/modules/gui/macosx/intf.m
> index c5fced6..398f8dc 100644
> --- a/modules/gui/macosx/intf.m
> +++ b/modules/gui/macosx/intf.m
> @@ -280,7 +280,7 @@ static void Run(intf_thread_t *p_intf)
> [[VLCMain sharedInstance] setIntf: p_intf];
>
> /* subscribe to LibVLCCore's messages */
> - vlc_LogSet(p_intf->p_libvlc, MsgCallback, NULL);
> + //vlc_LogSet(p_intf->p_libvlc, MsgCallback, NULL);
This should be moved to wherever the message dialog or MacOS equivalent gets
created, and then only after the callback data is initialized.
>
> [NSBundle loadNibNamed: @"MainMenu" owner: NSApp];
>
> @@ -769,6 +769,7 @@ static VLCMain *_o_sharedMainInstance = nil;
> [o_mainwindow updateWindow];
> [o_mainwindow updateTimeSlider];
> [o_mainwindow updateVolumeSlider];
> + msg_Dbg(p_intf, "applicationDidFinishLaunching");
Is this really going to teach you anything? Debug messages are very poor tool
to troubleshoot timing issues. Threaded backtrace on the other hand do not
clutter everybody's logs and far more helpful. Memory debugging tool as well.
> }
>
> - (void)initStrings
> @@ -799,6 +800,7 @@ static VLCMain *_o_sharedMainInstance = nil;
> {
> /* don't allow a double termination call. If the user has
> * already invoked the quit then simply return this time. */
> + msg_Dbg(p_intf, "applicationWillTerminate");
> static bool f_appExit = false;
> bool isTerminating;
>
> @@ -808,8 +810,10 @@ static VLCMain *_o_sharedMainInstance = nil;
> [o_appLock unlock];
>
> if (isTerminating)
> + {
> + msg_Dbg(p_intf, "already terminating - aborting
> applicationWillTerminate"); return;
> -
> + }
> [self resumeItunesPlayback:nil];
>
> if (notification == nil)
> @@ -818,6 +822,10 @@ static VLCMain *_o_sharedMainInstance = nil;
> playlist_t * p_playlist = pl_Get(p_intf);
> int returnedValue = 0;
>
> + // check if playlist has items and clear the playlist in order to
> prevent the playlist from starting again during shutdown process +
> msg_Dbg(p_intf, "deleting playlist");
> + [o_playlist deletePlaylistRoot:p_playlist];
> +
> /* always exit fullscreen on quit, otherwise we get ugly artifacts on
> the next launch */ if (b_nativeFullscreenMode) {
> [o_mainwindow toggleFullScreen: self];
> @@ -891,7 +899,7 @@ static VLCMain *_o_sharedMainInstance = nil;
> [o_eyetv release];
>
> /* unsubscribe from libvlc's debug messages */
> - vlc_LogSet(p_intf->p_libvlc, NULL, NULL);
> + //vlc_LogSet(p_intf->p_libvlc, NULL, NULL);
Should be done when closing the message dialog, before deleting its resources.
>
> [o_msg_arr removeAllObjects];
> [o_msg_arr release];
> @@ -1369,7 +1377,6 @@ static VLCMain *_o_sharedMainInstance = nil;
> [iTunesApp playpause];
> }
> }
> -
Please don't mix cosmetic stuff with serious fixes. Mind the reviewers.
> }
>
> b_has_itunes_paused = NO;
> @@ -1405,7 +1412,6 @@ static VLCMain *_o_sharedMainInstance = nil;
> }
> }
>
> -
> /* Declare user activity.
> This wakes the display if it is off, and postpones display sleep
> according to the users system preferences Available from 10.7.3 */
> @@ -2029,6 +2035,7 @@ static VLCMain *_o_sharedMainInstance = nil;
> // see [af97f24d528acab89969d6541d83f17ce1ecd580] that introduced the
> removal of setjmp() and longjmp() - (void)terminate:(id)sender
> {
> + msg_Dbg(VLCIntf, "VLCApplication terminate");
> [self activateIgnoringOtherApps:YES];
> [self stop:sender];
> }
> diff --git a/modules/gui/macosx/misc.m b/modules/gui/macosx/misc.m
> index 62779b1..9e3d7eb 100644
> --- a/modules/gui/macosx/misc.m
> +++ b/modules/gui/macosx/misc.m
> @@ -177,7 +177,8 @@ static NSMutableArray *blackoutWindows = NULL;
>
> for ( NSUInteger i = 0; i < count; i++ ) {
> NSScreen *screen = [[NSScreen screens] objectAtIndex: i];
> - if ([screen displayID] == displayID)
> + CGDirectDisplayID thisDisplayID = (CGDirectDisplayID)[[[screen
> deviceDescription] objectForKey: @"NSScreenNumber"] intValue];
> + if
> (thisDisplayID == displayID)
> return screen;
> }
> return nil;
> @@ -604,7 +605,6 @@ void _drawFrameInRect(NSRect frameRect)
> @"NO", @"DisplayTimeAsTimeRemaining",
> @"YES",
> @"DisplayFullscreenTimeAsTimeRemaining", nil];
> -
> [defaults registerDefaults:appDefaults];
> }
>
> @@ -614,7 +614,6 @@ void _drawFrameInRect(NSRect frameRect)
> textAlignment = NSCenterTextAlignment;
> o_remaining_identifier = @"";
> }
> -
> return self;
> }
>
> diff --git a/modules/gui/macosx/playlist.h b/modules/gui/macosx/playlist.h
> index 67c273b..d9c80cb 100644
> --- a/modules/gui/macosx/playlist.h
> +++ b/modules/gui/macosx/playlist.h
> @@ -127,6 +127,7 @@
> - (void)outlineViewSelectionDidChange:(NSNotification *)notification;
> - (void)sortNode:(int)i_mode;
> - (void)updateRowSelection;
> +- (void)deletePlaylistRoot:(playlist_t *)p_thePlaylist;
>
> - (BOOL)isSelectionEmpty;
>
> diff --git a/modules/gui/macosx/playlist.m b/modules/gui/macosx/playlist.m
> index c4360b8..7b0a390 100644
> --- a/modules/gui/macosx/playlist.m
> +++ b/modules/gui/macosx/playlist.m
> @@ -614,6 +614,14 @@
> [o_outline_view setNeedsDisplay:YES];
> }
>
> +- (void)deletePlaylistRoot:(playlist_t *)p_thePlaylist
> +{
> + playlist_t * p_playlist = p_thePlaylist;
> + PL_LOCK;
> + playlist_NodeDelete(p_playlist, [self currentPlaylistRoot], true,
> false); + PL_UNLOCK;
> +}
> +
> /* Check if p_item is a child of p_node recursively. We need to check the
> item existence first since OSX sometimes tries to redraw items that have
> been deleted. We don't do it when not required since this verification
> takes @@ -886,9 +894,7 @@
> #ifndef NDEBUG
> msg_Dbg(p_intf, "user selected entire list, deleting current
> playlist root instead of individual items"); #endif
> - PL_LOCK;
> - playlist_NodeDelete(p_playlist, [self currentPlaylistRoot], true,
> false); - PL_UNLOCK;
> + [self deletePlaylistRoot:p_playlist];
Seriously, you are not a complete rookie anymore. You need to think of the bit
bigger picture. You will never make great code if you stick to a pure
symptomatic analysis; health doctors do that, not developers.
Sure, deleting the playlist content is likely to stop the playback. But what
does that really warranty in the end? Does it really warrants whatever
assumptions you make afterward? That seems dubious.
> [self playlistUpdated];
> return;
> }
> diff --git a/src/playlist/thread.c b/src/playlist/thread.c
> index 9bfbc3c..733db1e 100644
> --- a/src/playlist/thread.c
> +++ b/src/playlist/thread.c
> @@ -155,7 +155,7 @@ void ResetCurrentlyPlaying( playlist_t *p_playlist,
> p_playlist->i_current_index = -1;
> for( playlist_item_t *p_next = NULL; ; )
> {
> - /** FIXME: this is *slow* */
> + /* FIXME: this is *slow* */
What is this? You should really review your patches before you send them.
> p_next = playlist_GetNextLeaf( p_playlist,
> p_sys->status.p_node,
> p_next, true, false );
--
Rémi Denis-Courmont
http://www.remlab.net/
More information about the vlc-devel
mailing list