[vlc-devel] [PATCH] macosx: First commit VLCStatusBarIcon code for macosx gui.

[User Goran]

<SNIP>

> 
> Hi Goran,
> 
> First, thanks for your contribution. I like the idea. The patch needs some improvements still, but I would be glad if we can merge your code in the end, after we improved it a bit.

Hi David,

Thanks. Hopefully it'll get integrated. It's not anything fancy, but I like
having the menu there.

> 
> I???ll do a more detailed review later one, but first some general remarks:
> - The first menu item looks a bit ugly, with those dashes if nothing is playing, and with the complete url if something is playing. Maybe we should discuss about the usefulness of this feature (who needs the complete url, which also starts with file:// for local media? How do you use this information? Maybe some other media summary is better?)

The point here was that you can copy and then paste your URL in the
next 'tool', like a stream URL to IRC (hey dude listen to this!)
in a split second :) No nead to find the main window, then the stream,
copy the right info.

Doing this for local files is still interesting, but only if you can open
the path/file through the finder or something. So you can immediately
scrap it if it sucks or rename it, for instance. I do see usecases. 

I did consider (and try) just saying 'Copy URL/PATH' in the item
and nothing else, as it is already apparent from the tooltip what
will be copied. So we can go there.

This was more of a 'why not, I can do it' kind of thing. Remember,
this is my first macosx project, so I'm trying out various options
in cocoa.


> - Activating the main window: should be renamed a bit, but might be useful indeed.

Rename to what, if I dare ask? It is called 'Main Window' in the regular
VLC 'Window' menu :) 

Mine's a bit better though, imho.. It shows the actual window in
front of everything else, in whatever state it was. You don't have
to activate the VLC menu bar (eg. through alt/tab or dock icon) and
then go to the window menu to get the main window (which is a crazy
mac-ism, if you ask me).

But calling it 'Restore Main Window' or 'Show Main Window' or even
'Summon Main Window' makes as much sense. It's up to you.

> - Play / Stop: Fine, but I do not like the icons that much. I think we should remove them, as this would be a more OS X like menu.

I like the visual cue. As on all devices ever, these actions are
represented by those icons. And lots of macosx apps have icons in their
menu's. It's a standard menuItem feature too, no trickery needed.

But I can live without them, if you think it fits better. There is no 
pressing need to keep them there.

> - Rest of the menu seems fine.
> - Tooltip: I would remove the URL from the list. Also, if nothing is playing, I would just write something like ???No media playing currently???, not listing all those items explicitly.

Sure, I can do that. Seems cleaner indeed.

> 
> Regarding the gathering of all information:
> Currently you are using a poll-based approach and the timer to update the tooltip periodically. I understand this is more easy at a first glance, but it is not ok for the final version.
> Polling is bad in general, and more importantly, information is always a bit out of date (I already saw the URL in the first menu item missing quite some times, even after the media started playing already).

It probably is, but I was aiming to change as little code as possible and
I came away with just a couple of changed lines in intf.h/m. 

That being said, my original intent was to get the info when entering the
icon area with the mouse, on demand.

This would never give a delay, and still keep the code 99% independent. But
it isn't possible in this form. Then the whole statusIcon and menu doodads
need to be replaced with a custom view and custom handling. Maybe later :-)

Obviously, if you accept this into the codebase, it would probably not be
an issue to update the relevant class variables from within the rest of
VLC in stead of polling.


> I think we need to implement a direct update of the tooltip once information changes. But we can help you with that.
> 
> Best regards,
> David
> 

Sure, again. If this thing is deamed useful and worthy of inclusion, then
there is no reason to keep on the sidelines. By all means call
VLCStatusBarIcon methods from the rest of the code. We can update the 
@properties or whatever else exposes the variables to 
other classes, as needed.

Let us see if any more comments come in for a couple days and then we can
decide the direction and solutions.

Thanks for the rundown and I'm glad you like the idea. And I enjoyed
figuring all this stuff out the last couple of days.

	goran