GNOME Bugzilla – Bug 324644
Maemo/Hildon UI Support
Last modified: 2009-10-26 12:18:19 UTC
Changes in the UI to run Evince on Maemo Platform.
Created attachment 56226 [details] [review] This patch contains all modifications necessary to make evince run on Maemo Platform. With this patch applied, the user who wants to compile evince on maemo platform just has to run configure with --enable-hildon option.
Created attachment 57741 [details] [review] Patch for version 0.5.0 An updated version of the patch for the last stable version (0.5.0)
Since this request does include a patch I think it is only fair to move it from Unconfirmed to New.
(In reply to comment #3) > Since this request does include a patch I think it is only fair to move it from > Unconfirmed to New. > The patch is attached, but i suspect it will not work for the current HEAD. I will attach a newer version this weekend.
Confirmed that it doens't apply with 0.5.2 or later.
Created attachment 67641 [details] [review] Partial update to 0.5.3 Attaching a partial update for Evince 0.5.3. The rejects are all in ev-window.c, which needs to be updated anyway now for Maemo 2.0.
The version of gtk+ in Maemo is 2.6.10. I tried to compile Evince 0.5.3, but it was not possible since it requires gtk+ 2.8. Newer versions now require gtk+ 2.10. So we'll have to stay in 0.5.2 until we don't have gtk+ 2.10 in Maemo. IMHO, it will not happen soon.
Created attachment 71101 [details] [review] Final Patch for 0.5.2 on Maemo 2.0
Created attachment 102469 [details] [review] Patch for 2.19.92 I've created this patch on Nov, 9th 2007. Don't know if it is still applicable. Sorry for taking so long to upload it here.
A few review comments: ++ ../evince-2.19.92-hildon/data/evince.service 2007-09-11 20:05:45.000000000 +0000 @@ -0,0 +1,3 @@ +[D-BUS Service] +Name=com.nokia.evince Why not org.gnome.Evince ? You just have to pass the full name "org.gnome.Evince" to osso_initialize instead of just the leaf name "evince". @@ -12,7 +12,8 @@ INCLUDES= \ $(WARN_CFLAGS) \ $(DISABLE_DEPRECATED) \ $(GNOME_PRINT_CFLAGS) \ - $(GTK_PRINT_CFLAGS) + $(GTK_PRINT_CFLAGS) \ + $(HILDON_CFLAGS) Should make that conditional on USE_HILDON. - gtk_widget_show (new_window); + gtk_widget_show_all (new_window); That's got to be wrong. What's this supposed to fix ? static void set_widget_visibility (GtkWidget *widget, gboolean visible) { - g_assert (GTK_IS_WIDGET (widget)); - - if (visible) - gtk_widget_show (widget); - else - gtk_widget_hide (widget); + if (widget) { + g_assert (GTK_IS_WIDGET (widget)); + + if (visible) + gtk_widget_show (widget); + else + gtk_widget_hide (widget); + } } What's this for ? - { "ViewZoomIn", GTK_STOCK_ZOOM_IN, NULL, "<control>plus", + { "ViewZoomIn", GTK_STOCK_ZOOM_IN, NULL, "ZOOM_IN_ACCEL", You want ZOOM_IN_ACCEL, not "ZOOM_IN_ACCEL" here :) I'd prefer if you'd just put the accels in the struct; and compiled the tooltips out since they're unneeded. For example, this is how I did it in aisleriot: #ifdef HAVE_HILDON /* We never show tooltips, no need to put them into the binary */ #define ACTION_TOOLTIP(string) (NULL) #define ACTION_ACCEL(string1,string2) (string2) #else #define ACTION_TOOLTIP(string) (string) #define ACTION_ACCEL(string1,string2) (string1) #endif { "Select", GTK_STOCK_INDEX, N_("_Select Game..."), ACTION_ACCEL ("<ctrl>O", NULL), ACTION_TOOLTIP (N_("Play a different game")), G_CALLBACK (select_game_cb) }, etc. + ui_file = g_build_filename (DATADIR, UI_FILE, NULL); if (!gtk_ui_manager_add_ui_from_file (ev_window->priv->ui_manager, - DATADIR"/evince-ui.xml", + ui_file, Just use DATADIR G_DIR_SEPARATOR_S UI_FILE instead of allocating the name? +#ifdef USE_HILDON + osso_context = osso_initialize (PACKAGE, VERSION, TRUE, NULL); +#endif Should pass the full service name instead of PACKAGE here, see above. --- - do menus have images, mnemonics or accels showing? - do buttons (in dialogues) show images ? - more code could probably be just compiled out, e.g. the whole of the printing code, unused GtkActions etc.
(In reply to comment #10) > A few review comments: > > ++ ../evince-2.19.92-hildon/data/evince.service 2007-09-11 20:05:45.000000000 > +0000 > @@ -0,0 +1,3 @@ > +[D-BUS Service] > +Name=com.nokia.evince > > Why not org.gnome.Evince ? You just have to pass the full name > "org.gnome.Evince" to osso_initialize instead of just the leaf name "evince". > Good point. This was inherited by the first version I ported to Maemo which was released for OS2005. > @@ -12,7 +12,8 @@ INCLUDES= \ > $(WARN_CFLAGS) \ > $(DISABLE_DEPRECATED) \ > $(GNOME_PRINT_CFLAGS) \ > - $(GTK_PRINT_CFLAGS) > + $(GTK_PRINT_CFLAGS) \ > + $(HILDON_CFLAGS) > > Should make that conditional on USE_HILDON. > Don't think so. HILDON_CFLAGS will be empty if we are not building for maemo so in my opinion, there is no need for a conditional here. > - gtk_widget_show (new_window); > + gtk_widget_show_all (new_window); > > That's got to be wrong. What's this supposed to fix ? > This is a issue with maemo gtk. Don't know if is fixed by now. Need to check. Anyway, there should be a #ifdef in this piece of code. > static void > set_widget_visibility (GtkWidget *widget, gboolean visible) > { > - g_assert (GTK_IS_WIDGET (widget)); > - > - if (visible) > - gtk_widget_show (widget); > - else > - gtk_widget_hide (widget); > + if (widget) { > + g_assert (GTK_IS_WIDGET (widget)); > + > + if (visible) > + gtk_widget_show (widget); > + else > + gtk_widget_hide (widget); > + } > } > > What's this for ? AFAIR, this is to avoid crash or even the assertion to fail. Some widgets, like the status bar, for instance won't be created if compiling for Maemo. > > - { "ViewZoomIn", GTK_STOCK_ZOOM_IN, NULL, "<control>plus", > + { "ViewZoomIn", GTK_STOCK_ZOOM_IN, NULL, "ZOOM_IN_ACCEL", > > You want ZOOM_IN_ACCEL, not "ZOOM_IN_ACCEL" here :) Indeed. > I'd prefer if you'd just put the accels in the struct; and compiled the > tooltips out since they're unneeded. For example, this is how I did it in > aisleriot: > > #ifdef HAVE_HILDON > /* We never show tooltips, no need to put them into the binary */ > #define ACTION_TOOLTIP(string) (NULL) > #define ACTION_ACCEL(string1,string2) (string2) > #else > #define ACTION_TOOLTIP(string) (string) > #define ACTION_ACCEL(string1,string2) (string1) > #endif > > { "Select", GTK_STOCK_INDEX, N_("_Select Game..."), > ACTION_ACCEL ("<ctrl>O", NULL), > ACTION_TOOLTIP (N_("Play a different game")), > G_CALLBACK (select_game_cb) }, > > etc. > Really good idea. I'll probably use it. > + ui_file = g_build_filename (DATADIR, UI_FILE, NULL); > if (!gtk_ui_manager_add_ui_from_file (ev_window->priv->ui_manager, > - DATADIR"/evince-ui.xml", > + ui_file, > > Just use DATADIR G_DIR_SEPARATOR_S UI_FILE instead of allocating the name? > Yeah, I definitely should have done this... :) > --- > > - do menus have images, mnemonics or accels showing? > - do buttons (in dialogues) show images ? It's just a matter of setting GtkWindow GtkSettings properties "gtk-button-images" and "gtk-menu-images" to FALSE. > - more code could probably be just compiled out, e.g. the whole of the printing > code, unused GtkActions etc. >
(In reply to comment #11) > > @@ -12,7 +12,8 @@ INCLUDES= \ > > $(WARN_CFLAGS) \ > > $(DISABLE_DEPRECATED) \ > > $(GNOME_PRINT_CFLAGS) \ > > - $(GTK_PRINT_CFLAGS) > > + $(GTK_PRINT_CFLAGS) \ > > + $(HILDON_CFLAGS) > > > > Should make that conditional on USE_HILDON. > > > > Don't think so. HILDON_CFLAGS will be empty if we are not building for maemo so > in my opinion, there is no need for a conditional here. Ok. > > - gtk_widget_show (new_window); > > + gtk_widget_show_all (new_window); > > > > That's got to be wrong. What's this supposed to fix ? > > > > This is a issue with maemo gtk. Don't know if is fixed by now. Need to check. > Anyway, there should be a #ifdef in this piece of code. Probably maemo#615 and maemo#875 . > > static void > > set_widget_visibility (GtkWidget *widget, gboolean visible) > > { > > - g_assert (GTK_IS_WIDGET (widget)); > > - > > - if (visible) > > - gtk_widget_show (widget); > > - else > > - gtk_widget_hide (widget); > > + if (widget) { > > + g_assert (GTK_IS_WIDGET (widget)); > > + > > + if (visible) > > + gtk_widget_show (widget); > > + else > > + gtk_widget_hide (widget); > > + } > > } > > > > What's this for ? > > AFAIR, this is to avoid crash or even the assertion to fail. Some widgets, like > the status bar, for instance won't be created if compiling for Maemo. In that case I think you should #ifdef the code setting its visibility, not make this function NULL-safe. > > - do menus have images, mnemonics or accels showing? > > - do buttons (in dialogues) show images ? > > It's just a matter of setting GtkWindow GtkSettings properties > "gtk-button-images" and "gtk-menu-images" to FALSE. I know. I was asking because I don't see the code doing that :)
(In reply to comment #12) > > > - gtk_widget_show (new_window); > > > + gtk_widget_show_all (new_window); > > > > > > That's got to be wrong. What's this supposed to fix ? > > > > > > > This is a issue with maemo gtk. Don't know if is fixed by now. Need to check. > > Anyway, there should be a #ifdef in this piece of code. > > Probably maemo#615 and maemo#875 . > Exactly. Good to know these issues are fixed. I'll remove it in the next version. > > > static void > > > set_widget_visibility (GtkWidget *widget, gboolean visible) > > > { > > > - g_assert (GTK_IS_WIDGET (widget)); > > > - > > > - if (visible) > > > - gtk_widget_show (widget); > > > - else > > > - gtk_widget_hide (widget); > > > + if (widget) { > > > + g_assert (GTK_IS_WIDGET (widget)); > > > + > > > + if (visible) > > > + gtk_widget_show (widget); > > > + else > > > + gtk_widget_hide (widget); > > > + } > > > } > > > > > > What's this for ? > > > > AFAIR, this is to avoid crash or even the assertion to fail. Some widgets, like > > the status bar, for instance won't be created if compiling for Maemo. > > In that case I think you should #ifdef the code setting its visibility, not > make this function NULL-safe. > Ok. > > > - do menus have images, mnemonics or accels showing? > > > - do buttons (in dialogues) show images ? > > > > It's just a matter of setting GtkWindow GtkSettings properties > > "gtk-button-images" and "gtk-menu-images" to FALSE. > I know. I was asking because I don't see the code doing that :) > Hehehe... Next patch will include it then.
Hey Eduardo, really nice to know you are working on this again :-)) You should also disable the images extraction, it's not very recommendable for devices with limited resources like Nokia N*. It should be enough to #ifdef this code (in shell/ev-jobs.c: ev_job_render_run) if (job->include_images && EV_IS_DOCUMENT_IMAGES (EV_JOB (job)->document)) job->image_mapping = ev_document_images_get_images (EV_DOCUMENT_IMAGES (EV_JOB (job)->document), job->rc->page);
(In reply to comment #14) > Hey Eduardo, really nice to know you are working on this again :-)) > Hey Kal! Well, It's not me precisely working on this. I've talked to one of our trainees if he'd like the idea of working on the patches of GNOME apps for Maemo. He loved the idea is working on it since last week. I'm kind of mentoring and reviewing his work on this task. > You should also disable the images extraction, it's not very recommendable for > devices with limited resources like Nokia N*. It should be enough to #ifdef > this code (in shell/ev-jobs.c: ev_job_render_run) > > if (job->include_images && EV_IS_DOCUMENT_IMAGES (EV_JOB (job)->document)) > job->image_mapping = > ev_document_images_get_images > (EV_DOCUMENT_IMAGES (EV_JOB (job)->document), > job->rc->page); > Thanks for the tip. The patch is coming soon. :)
Created attachment 103175 [details] [review] Patch for trunk.
With the previous patch, I've configured the package for compilation with ./autogen.sh --enable-hildon --disable-scrollkeeper --disable-thumbnailer --without-libgnome --without-print --without-keyring And have to compile gnome-doc-utils and libpoppler (got the source packages from gutsy) on scratchbox before this.
Seems evince.hildon.desktop.in.in and evince-hildon-ui.xml are both missing ?
>< I forgot to svn add those files. Thanks for pointing this out. They are on my computer at the office, so monday morning I will send the correct patch. Sorry for this. :(
Created attachment 103338 [details] [review] Patch for trunk. Here is the correct (in relation to my previous one) patch. I checked it twice, if it is still wrong I will call a doctor. :P
Created attachment 136068 [details] [review] patch series for master I started a new port of evince master to maemo5 beta, from scratch using only some bits from the latest patch from the maemo repos (the evince-hildon-ui.xml file mostly). A couple TODOs: - should we replace the GtkScrolledWindow:s in the sidebar and the main document view with HildonPannableArea:s ? - should crash recovery be disabled on the device? (And if no, fix the "Don't Recover button is not shown in the dialogue" problem) - react to the osso hardware events? e.g. clear pixbuf cache on memory pressure, save metadata when told to save data,...? - enable metadata ? (it's #ifdef ENABLE_DBUS which is off on hildon since osso does the single instance for us) And a couple of questions for the original patch authors (I took a look at the latest patch from the maemo repos, and my patch has things mostly covered, except a few chunks): - what is the "special" mode handling in EvWindow supposed to do? Should it be a global toggle, or per EvWindow? - what does the code in ev_view_button_release_event() do? will this be obsolete with HildonPannableArea? - what was the intent being "job->include_images = FALSE" ?
(In reply to comment #21) > - what was the intent being "job->include_images = FALSE" ? > I think this was a workaround for bug #504913
I pushed the configure bits to master already, to make branch maintenance easier for me.
Created attachment 137959 [details] [review] updated patch series to master
So what should we do with this port? - keep in my private branch, or - create public branch in gnome git, or - just directly merge to master?
hmm, I don't like #ifdefs, but having a separate branch would probably make things harder to maintain. Christian, are you going to continue maintaining the port? what do you prefer, then?
I too think the ifdefs are ugly :) Given that maemo5 will be the last gtk+ maemo, I don't really see a future for evince on maemo, so IMO committing this work only to a public branch but not master would be fine.
I pushed this to the hildon-2-28 branch. It's based on the gnome-2-28 branch since I think that for use on the maemo5 device, it's better to use a stable release. It'll later be ported to master/gnome-2-30 as necessary, but that's going to be a bit of an effort since master moved to a one-process-per-document model while hildon apps are one-process-only. If someone wants to package this for maemo5, it would be good to a) merge gnome-2-28 HEAD to hildon-2-28 before "make dist" or "make distcheck", so you get the latest bug fixes and translation updates, and b) to put the debian/ directory also on the hildon-2-28 branch. Closing as FIXED; if there are any issues remaining, please open new bugs, tag them with [maemo] in the subject and Status Whiteboard fields, and CC: me.