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

[Arnon Gilboa]

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)
> <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
> <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.

>
> <snip>
>
> Regards,
>
> Hans
Thanks,
Arnon
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel