[Spice-devel] [PATCH spice 8/9] spicec: add foreign menu

[Hans de Goede]

Hi,

On 10/18/2010 09:00 AM, Arnon Gilboa wrote:
> Hans de Goede wrote:
>> Hi,
>>
>> See 1 comment inline.
>>
>>>
>>> int Application::run()
>>> {
>>> + _exit_code = ProcessLoop::run();
>>> + return _exit_code;
>>> +}
>>> +
>>> +void Application::on_start_running()
>>> +{
>>> + _foreign_menu.reset(new ForeignMenu(this));
>>> #ifdef USE_GUI
>>> if (_gui_mode != GUI_MODE_FULL) {
>>> connect();
>>
>>
>> Erm, and who is going to call on_start_running, so that the
>> flow in the normal cmdline usage is not changed?
>>
> ProcessLoop::run calls n_start_running() before entering the main wait_events loop (see patch 3)

Oops, note to self don't do reviews when tired :)

>> <snip
>>>
>>> class Application : public ProcessLoop,
>>> public Platform::EventListener,
>>> public Platform::DisplayModeListener,
>>> - public CommandTarget {
>>> + public ForeignMenuInterface {
>>> public:
>>>
>>> enum State {
>>
>> You're loosing CommandTarget as class Application inherits
>> from here, is that right?
>>
> no, class ForeignMenuInterface : public CommandTarget

Ah, ok.

>> <snip>
>>
>>> diff --git a/client/foreign_menu_prot.h b/client/foreign_menu_prot.h
>>> new file mode 100644
>>> index 0000000..8c22461
>>> --- /dev/null
>>> +++ b/client/foreign_menu_prot.h
>>> @@ -0,0 +1,107 @@
>>> +/*
>>> + Copyright (C) 2009 Red Hat, Inc.
>>> +
>>> + This library is free software; you can redistribute it and/or
>>> + modify it under the terms of the GNU Lesser General Public
>>> + License as published by the Free Software Foundation; either
>>> + version 2.1 of the License, or (at your option) any later version.
>>> +
>>> + This library is distributed in the hope that it will be useful,
>>> + but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>>> + Lesser General Public License for more details.
>>> +
>>> + You should have received a copy of the GNU Lesser General Public
>>> + License along with this library; if not, see<http://www.gnu.org/licenses/>.
>>> +*/
>>> +
>>> +#ifndef _H_FOREIGN_MENU_PROT
>>> +#define _H_FOREIGN_MENU_PROT
>>> +
>>> +#define FOREIGN_MENU_MAGIC (*(uint32_t*)"FRGM")
>>> +#define FOREIGN_MENU_VERSION 1
>>> +
>>> +#ifdef __GNUC__
>>> +#define ATTR_PACKED __attribute__ ((__packed__))
>>> +#else
>>> +#define ATTR_PACKED __declspec(align(1))
>>> +#endif
>>> +
>>> +typedef struct ATTR_PACKED FrgMenuInitHeader {
>>> + uint32_t magic;
>>> + uint32_t version;
>>> + uint32_t size;
>>> +} FrgMenuInitHeader;
>>> +
>>> +typedef struct ATTR_PACKED FrgMenuInit {
>>> + FrgMenuInitHeader base;
>>> + uint64_t credentials;
>>> + uint8_t title[0]; //UTF8
>>> +} FrgMenuInit;
>>> +
>>> +typedef struct ATTR_PACKED FrgMenuMsg {
>>> + uint32_t id;
>>> + uint32_t size;
>>> +} FrgMenuMsg;
>>> +
>>> +enum {
>>> + //extrenal app -> spice client
>>> + FOREIGN_MENU_SET_TITLE = 1,
>>> + FOREIGN_MENU_ADD_ITEM,
>>> + FOREIGN_MENU_MODIFY_ITEM,
>>> + FOREIGN_MENU_REMOVE_ITEM,
>>> + FOREIGN_MENU_CLEAR,
>>> +
>>> + //spice client -> external app
>>> + FOREIGN_MENU_ITEM_EVENT = 1001,
>>> + FOREIGN_MENU_APP_ACTIVATED,
>>> + FOREIGN_MENU_APP_DEACTIVATED,
>>> +};
>>> +
>>> +typedef struct ATTR_PACKED FrgMenuSetTitle {
>>> + FrgMenuMsg base;
>>> + uint8_t string[0]; //UTF8
>>> +} FrgMenuSetTitle;
>>> +
>>> +enum {
>>> + FOREIGN_MENU_ITEM_TYPE_CHECKED = 1<< 0,
>>> + FOREIGN_MENU_ITEM_TYPE_DIM = 1<< 1,
>>> + FOREIGN_MENU_ITEM_TYPE_SEPARATOR = 1<< 2
>>> +};
>>> +
>>> +#define FOREIGN_MENU_INVALID_ID 0
>>> +
>>> +typedef struct ATTR_PACKED FrgMenuAddItem {
>>> + FrgMenuMsg base;
>>> + uint32_t id;
>>> + uint32_t type;
>>> + uint32_t position;
>>> + uint8_t string[0]; //UTF8
>>> +} FrgMenuAddItem, FrgMenuModItem;
>>> +
>>> +typedef struct ATTR_PACKED FrgMenuRmItem {
>>> + FrgMenuMsg base;
>>> + uint32_t id;
>>> +} FrgMenuRmItem;
>>> +
>>> +typedef struct FrgMenuMsg FrgMenuRmItems;
>>> +typedef struct FrgMenuMsg FrgMenuDelete;
>>> +
>>> +enum {
>>> + FOREIGN_MENU_EVENT_CLICK,
>>> + FOREIGN_MENU_EVENT_CHECKED,
>>> + FOREIGN_MENU_EVENT_UNCHECKED
>>> +};
>>> +
>>> +typedef struct ATTR_PACKED FrgMenuEvent {
>>> + FrgMenuMsg base;
>>> + uint32_t id;
>>> + uint32_t action; //FOREIGN_MENU_EVENT_?
>>> +} FrgMenuEvent;
>>> +
>>> +typedef struct FrgMenuMsg FrgMenuActivate;
>>> +typedef struct FrgMenuMsg FrgMenuDeactivate;
>>> +
>>> +#undef ATTR_PACKED
>>> +
>>> +#endif
>>
>> Hmm, this feels like it should be in spice-protocol not in spice!
> I was discussing it with Uri. It's client-side only, so I don't know if spice-protocol is better.
> However, you are right in that this enables building the xpi/activex independently, without the need to sync the headers.
>

So ack then except for this bit.

Regards,

Hans