GNOME.org
 

[glom] Image loading speedup

commit b6e97d1b1e9104b72de5a38948a320f27cecb14d
Author: Armin Burgmeier <armin arbur net>
Date:   Fri May 1 13:25:24 2009 +0200

    Image loading speedup
    
    	* glom/libglom/sharedptr.h: Added operator!=.
    
    	* glom/mode_data/flowtablewithfields.h:
    	* glom/mode_data/flowtablewithfields.cc: Added set_other_field_value
    	which is the same as set_field_value except that it does not set the
    	value for widget that belongs to the passed layout item's widget
    	itself. This can be used if that very widget already contains the new
    	value to avoid setting it again. Especially when dealing with large
    	images this brings an essential speedup.
    
    	* glom/mode_data/box_data_details.cc (on_flowtable_field_edited): Use
    	set_other_field_value, so that we don't set the value for the field
    	which the user already changed again.
    
    	* glom/utility_widgets/imageglom.h:
    	* glom/utility_widgets/imageglom.cc: Store the original data of the
    	image file, and return it in get_value(), instead of creating a PNG
    	from the raw image data, to speed up loading a large image file.
    
    	* glom/utils_ui.cc (get_pixbuf_for_gda_value): When loading images
    	from the database, allow all image types, not just PNGs.
    
    	* glom/xsl_utils.cc: Include gtk/gtk.h to fix the build for me.
---
 ChangeLog                             |   26 +++++++++
 glom/base_db.cc                       |    3 +-
 glom/libglom/sharedptr.h              |    7 +++
 glom/mode_data/box_data_details.cc    |    4 +-
 glom/mode_data/flowtablewithfields.cc |   85 +++++++++++++++++++++----------
 glom/mode_data/flowtablewithfields.h  |    8 ++-
 glom/utility_widgets/imageglom.cc     |   90 ++++++++++++++++++---------------
 glom/utility_widgets/imageglom.h      |    1 +
 glom/utils_ui.cc                      |    2 +-
 glom/xsl_utils.cc                     |    1 +
 10 files changed, 152 insertions(+), 75 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 2b4b77e..9fd2d39 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,29 @@
+2009-05-05  Armin Burgmeier  <armin arbur net>
+
+	* glom/libglom/sharedptr.h: Added operator!=.
+
+	* glom/mode_data/flowtablewithfields.h:
+	* glom/mode_data/flowtablewithfields.cc: Added set_other_field_value
+	which is the same as set_field_value except that it does not set the
+	value for widget that belongs to the passed layout item's widget
+	itself. This can be used if that very widget already contains the new
+	value to avoid setting it again. Especially when dealing with large
+	images this brings an essential speedup.
+
+	* glom/mode_data/box_data_details.cc (on_flowtable_field_edited): Use
+	set_other_field_value, so that we don't set the value for the field
+	which the user already changed again.
+
+	* glom/utility_widgets/imageglom.h:
+	* glom/utility_widgets/imageglom.cc: Store the original data of the
+	image file, and return it in get_value(), instead of creating a PNG
+	from the raw image data, to speed up loading a large image file.
+
+	* glom/utils_ui.cc (get_pixbuf_for_gda_value): When loading images
+	from the database, allow all image types, not just PNGs.
+
+	* glom/xsl_utils.cc: Include gtk/gtk.h to fix the build for me.
+
 2009-04-24  Murray Cumming  <murrayc murrayc com>
 
 	Don't network share new documents from (old) examples.
diff --git a/glom/base_db.cc b/glom/base_db.cc
index c7709a5..3c76544 100644
--- a/glom/base_db.cc
+++ b/glom/base_db.cc
@@ -2493,7 +2493,8 @@ bool Base_DB::set_field_value_in_database(const LayoutFieldInRecord& layoutfield
   { 
     Glib::RefPtr<Gnome::Gda::Set> params = Gnome::Gda::Set::create();
     params->add_holder(field_in_record.m_field->get_holder(field_value));
-    params->add_holder(field_in_record.m_key->get_holder(field_in_record.m_key_value));  
+    params->add_holder(field_in_record.m_key->get_holder(field_in_record.m_key_value));
+
     Glib::ustring strQuery = "UPDATE \"" + field_in_record.m_table_name + "\"";
     strQuery += " SET \"" + field_in_record.m_field->get_name() + "\" = " + field_in_record.m_field->get_gda_holder_string();
     strQuery += " WHERE \"" + field_in_record.m_key->get_name() + "\" = " + field_in_record.m_key->get_gda_holder_string();
diff --git a/glom/libglom/sharedptr.h b/glom/libglom/sharedptr.h
index eb6f1a8..f3082f9 100644
--- a/glom/libglom/sharedptr.h
+++ b/glom/libglom/sharedptr.h
@@ -79,6 +79,7 @@ public:
   virtual ~sharedptr();
 
   inline bool operator==(const sharedptr<T_obj>& src) const;
+  inline bool operator!=(const sharedptr<T_obj>& src) const;
 
   ///Forget the instance.
   virtual void clear();
@@ -268,6 +269,12 @@ inline bool sharedptr<T_obj>::operator==(const sharedptr<T_obj>& src) const
   return m_pobj == src.m_pobj;
 }
 
+template <class T_obj>
+inline bool sharedptr<T_obj>::operator!=(const sharedptr<T_obj>& src) const
+{
+  return m_pobj != src.m_pobj;
+}
+
 
 template< typename T_obj>
 void sharedptr<T_obj>::clear()
diff --git a/glom/mode_data/box_data_details.cc b/glom/mode_data/box_data_details.cc
index 11cd341..b7917e4 100644
--- a/glom/mode_data/box_data_details.cc
+++ b/glom/mode_data/box_data_details.cc
@@ -760,8 +760,8 @@ void Box_Data_Details::on_flowtable_field_edited(const sharedptr<const LayoutIte
     }
 
     //Set the value in all instances of this field in the layout (The field might be on the layout more than once):
-    //if(layout_field->get_glom_type() != Field::TYPE_IMAGE) //TODO For now, don't do this for images, because the ImageGlom widget expects a broken GdaValue, because gda_value_get_binary() needs to be fixed.
-    m_FlowTable.set_field_value(layout_field, field_value);
+    //We don't need to set the value in the layout_field itself, as this is where the value comes from.
+    m_FlowTable.set_other_field_value(layout_field, field_value);
 
     //Update the field in the record (the record with this primary key):
 
diff --git a/glom/mode_data/flowtablewithfields.cc b/glom/mode_data/flowtablewithfields.cc
index 71f1546..93fa18a 100644
--- a/glom/mode_data/flowtablewithfields.cc
+++ b/glom/mode_data/flowtablewithfields.cc
@@ -742,8 +742,36 @@ void FlowTableWithFields::remove_field(const Glib::ustring& id)
 
 void FlowTableWithFields::set_field_value(const sharedptr<const LayoutItem_Field>& field, const Gnome::Gda::Value& value)
 {
+  // TODO: Avoid duplication. Maybe we can call set_other_field_value plus
+  // set the value for field's widget directly.
   //Set widgets which should show the value of this field:
-  type_list_widgets list_widgets = get_field(field);
+  type_list_widgets list_widgets = get_field(field, true);
+  for(type_list_widgets::iterator iter = list_widgets.begin(); iter != list_widgets.end(); ++iter)
+  {
+    DataWidget* datawidget = dynamic_cast<DataWidget*>(*iter);
+    if(datawidget)
+    {
+      datawidget->set_value(value);
+    }
+  }
+
+  //Refresh widgets which should show the related records for relationships that use this field:
+  type_portals list_portals = get_portals(field /* from_key field name */);
+  for(type_portals::iterator iter = list_portals.begin(); iter != list_portals.end(); ++iter)
+  {
+    Box_Data_Portal* portal = *iter;
+    if(portal)
+    {
+      //g_warning("FlowTableWithFields::set_field_value: foreign_key_value=%s", value.to_string().c_str());
+      portal->refresh_data_from_database_with_foreign_key(value /* foreign key value */);
+    }
+  }
+}
+
+void FlowTableWithFields::set_other_field_value(const sharedptr<const LayoutItem_Field>& field, const Gnome::Gda::Value& value)
+{
+  //Set widgets which should show the value of this field:
+  type_list_widgets list_widgets = get_field(field, false);
   for(type_list_widgets::iterator iter = list_widgets.begin(); iter != list_widgets.end(); ++iter)
   {
     DataWidget* datawidget = dynamic_cast<DataWidget*>(*iter);
@@ -768,7 +796,7 @@ void FlowTableWithFields::set_field_value(const sharedptr<const LayoutItem_Field
 
 Gnome::Gda::Value FlowTableWithFields::get_field_value(const sharedptr<const LayoutItem_Field>& field) const
 {
-  type_list_const_widgets list_widgets = get_field(field);
+  type_list_const_widgets list_widgets = get_field(field, true);
   if(!list_widgets.empty())
   {
     const DataWidget* datawidget = dynamic_cast<const DataWidget*>(*(list_widgets.begin()));
@@ -783,7 +811,7 @@ Gnome::Gda::Value FlowTableWithFields::get_field_value(const sharedptr<const Lay
 
 void FlowTableWithFields::set_field_editable(const sharedptr<const LayoutItem_Field>& field, bool editable)
 {
-  type_list_widgets list_widgets = get_field(field);
+  type_list_widgets list_widgets = get_field(field, true);
   for(type_list_widgets::iterator iter = list_widgets.begin(); iter != list_widgets.end(); ++iter)
   {
     DataWidget* datawidget = dynamic_cast<DataWidget*>(*iter);
@@ -840,21 +868,31 @@ FlowTableWithFields::type_portals FlowTableWithFields::get_portals(const sharedp
   return result;
 }
 
-FlowTableWithFields::type_list_const_widgets FlowTableWithFields::get_field(const sharedptr<const LayoutItem_Field>& layout_item) const
+namespace
 {
-  type_list_const_widgets result;
-
-  for(type_listFields::const_iterator iter = m_listFields.begin(); iter != m_listFields.end(); ++iter)
+  // Get the direct widgets represesenting a given layout item
+  // from a flowtable, without considering subflowtables:
+  template<typename InputIterator, typename OutputIterator>
+  void get_direct_fields(InputIterator begin, InputIterator end, OutputIterator out, const sharedptr<const LayoutItem_Field>& layout_item, bool include_item)
   {
-    if( (iter->m_field->is_same_field(layout_item)) )
+    for(InputIterator iter = begin; iter != end; ++iter)
     {
-      const Info& info = *iter;
-      if(info.m_checkbutton)
-        result.push_back(info.m_checkbutton);
-      else
-        result.push_back(info.m_second);
+      if(iter->m_field->is_same_field(layout_item) && (include_item || iter->m_field != layout_item))
+      {
+        if(iter->m_checkbutton)
+          *out++ = iter->m_checkbutton;
+        else
+          *out++ = iter->m_second;
+      }
     }
   }
+}
+
+FlowTableWithFields::type_list_const_widgets FlowTableWithFields::get_field(const sharedptr<const LayoutItem_Field>& layout_item, bool include_item) const
+{
+  type_list_const_widgets result;
+
+  get_direct_fields(m_listFields.begin(), m_listFields.end(), std::back_inserter(result), layout_item, include_item);
 
   //Check the sub-flowtables:
   for(type_sub_flow_tables::const_iterator iter = m_sub_flow_tables.begin(); iter != m_sub_flow_tables.end(); ++iter)
@@ -862,7 +900,7 @@ FlowTableWithFields::type_list_const_widgets FlowTableWithFields::get_field(cons
     const FlowTableWithFields* subtable = *iter;
     if(subtable)
     {
-      type_list_const_widgets sub_list = subtable->get_field(layout_item);
+      type_list_const_widgets sub_list = subtable->get_field(layout_item, include_item);
       if(!sub_list.empty())
       {
         //Add to the main result:
@@ -874,22 +912,13 @@ FlowTableWithFields::type_list_const_widgets FlowTableWithFields::get_field(cons
   return result;
 }
 
-FlowTableWithFields::type_list_widgets FlowTableWithFields::get_field(const sharedptr<const LayoutItem_Field>& layout_item)
+FlowTableWithFields::type_list_widgets FlowTableWithFields::get_field(const sharedptr<const LayoutItem_Field>& layout_item, bool include_item)
 {
-  //TODO: Avoid duplication
   type_list_widgets result;
 
-  for(type_listFields::const_iterator iter = m_listFields.begin(); iter != m_listFields.end(); ++iter)
-  {
-    if( (iter->m_field->is_same_field(layout_item)) )
-    {
-      const Info& info = *iter;
-      if(info.m_checkbutton)
-        result.push_back(info.m_checkbutton);
-      else
-        result.push_back(info.m_second);
-    }
-  }
+  get_direct_fields(m_listFields.begin(), m_listFields.end(), std::back_inserter(result), layout_item, include_item);
+
+  //TODO: Avoid duplication
 
   //Check the sub-flowtables:
   for(type_sub_flow_tables::const_iterator iter = m_sub_flow_tables.begin(); iter != m_sub_flow_tables.end(); ++iter)
@@ -897,7 +926,7 @@ FlowTableWithFields::type_list_widgets FlowTableWithFields::get_field(const shar
     FlowTableWithFields* subtable = *iter;
     if(subtable)
     {
-      type_list_widgets sub_list = subtable->get_field(layout_item);
+      type_list_widgets sub_list = subtable->get_field(layout_item, include_item);
       if(!sub_list.empty())
       {
         //Add to the main result:
diff --git a/glom/mode_data/flowtablewithfields.h b/glom/mode_data/flowtablewithfields.h
index a7e9d3a..d7af426 100644
--- a/glom/mode_data/flowtablewithfields.h
+++ b/glom/mode_data/flowtablewithfields.h
@@ -87,7 +87,7 @@ public:
   //virtual Gnome::Gda::Value get_field_value(const Glib::ustring& id) const;
   virtual void set_field_value(const sharedptr<const LayoutItem_Field>& field, const Gnome::Gda::Value& value);
   //virtual void set_field_value(const Glib::ustring& id, const Gnome::Gda::Value& value);
-
+  virtual void set_other_field_value(const sharedptr<const LayoutItem_Field>& field, const Gnome::Gda::Value& value);
 
   typedef std::list<Gtk::Widget*> type_list_widgets;
   typedef std::list<const Gtk::Widget*> type_list_const_widgets;
@@ -138,8 +138,10 @@ public:
   
 private:
 
-  type_list_widgets get_field(const sharedptr<const LayoutItem_Field>& field);
-  type_list_const_widgets get_field(const sharedptr<const LayoutItem_Field>& field) const;
+  // If include_item is set, then the output list will contain field's widget,
+  // otherwise not.
+  type_list_widgets get_field(const sharedptr<const LayoutItem_Field>& field, bool include_item);
+  type_list_const_widgets get_field(const sharedptr<const LayoutItem_Field>& field, bool include_item) const;
 
   typedef std::list< Box_Data_Portal* > type_portals;
     
diff --git a/glom/utility_widgets/imageglom.cc b/glom/utility_widgets/imageglom.cc
index 561ed39..384a7f9 100644
--- a/glom/utility_widgets/imageglom.cc
+++ b/glom/utility_widgets/imageglom.cc
@@ -157,7 +157,11 @@ void ImageGlom::set_pixbuf(const Glib::RefPtr<Gdk::Pixbuf>& pixbuf)
 
 void ImageGlom::set_value(const Gnome::Gda::Value& value)
 {
+  // Remember original data 
+  m_original_data = Gnome::Gda::Value();
+  m_original_data = value;
   Glib::RefPtr<Gdk::Pixbuf> pixbuf = Utils::get_pixbuf_for_gda_value(value);
+
   if(pixbuf)
   {
     set_pixbuf(pixbuf);
@@ -219,7 +223,8 @@ Gnome::Gda::Value ImageGlom::get_value() const
 {
   //TODO: Return the data from the file that was just chosen.
   //Don't store the original here any longer than necessary,
-  Gnome::Gda::Value result;
+  if(m_original_data.get_value_type() != G_TYPE_NONE)
+    return m_original_data;
   
   if(m_pixbuf_original)
   {
@@ -245,11 +250,16 @@ Gnome::Gda::Value ImageGlom::get_value() const
       //for(int i = 0; i < 10; ++i)
       //  g_warning("%02X (%c), ", (guint8)buffer[i], buffer[i]);
 
-      result.Glib::ValueBase::init(GDA_TYPE_BINARY);
-      result.set(reinterpret_cast<const guchar*>(buffer), buffer_size);
+      GdaBinary* bin = g_new(GdaBinary, 1);
+      bin->data = reinterpret_cast<guchar*>(buffer);
+      bin->binary_length = buffer_size;
+
+      m_original_data = Gnome::Gda::Value();
+      m_original_data.Glib::ValueBase::init(GDA_TYPE_BINARY);
+      gda_value_take_binary(m_original_data.gobj(), bin);
 
-      g_free(buffer);
       buffer = 0;
+      return m_original_data;
     }
 #ifdef GLIBMM_EXCEPTIONS_ENABLED
     catch(const Glib::Exception& ex)
@@ -259,7 +269,7 @@ Gnome::Gda::Value ImageGlom::get_value() const
 #endif // GLIBMM_EXCEPTIONS_ENABLED
   }
 
-  return result;
+  return Gnome::Gda::Value();
 }
 
 bool ImageGlom::on_expose_event(GdkEventExpose* event)
@@ -398,44 +408,41 @@ void ImageGlom::on_menupopup_activate_select_file()
     const std::string filepath = dialog.get_filename();
     if(!filepath.empty())
     {
-#ifdef GLIBMM_EXCEPTIONS_ENABLED
-      try
-#else
-      std::auto_ptr<Glib::Error> error;
-#endif // GLIBMM_EXCEPTIONS_ENABLED
-      {
-#ifdef GLIBMM_EXCEPTIONS_ENABLED
-        m_pixbuf_original = Gdk::Pixbuf::create_from_file(filepath);
-#else
-        m_pixbuf_original = Gdk::Pixbuf::create_from_file(filepath, error);
-        if(error.get() != NULL)
-        {
-#endif // GLIBMM_EXCEPTIONS_ENABLED
-          if(m_pixbuf_original)
-          {
-            m_image.set(m_pixbuf_original); //Load the image.
-            scale();
-            signal_edited().emit();
-          }
-          else
-          {
-            g_warning("ImageGlom::on_menupopup_activate_select_file(): file load failed.");
-          }
-#ifndef GLIBMM_EXCEPTIONS_ENABLED
-        }
-#endif // !GLIBMM_EXCEPTIONS_ENABLED
-      }
-#ifdef GLIBMM_EXCEPTIONS_ENABLED
-      catch(const Glib::Exception& ex)
+      // TODO: We might want to show a progress dialog here, and read
+      // the data asynchronously
+      // We use the C API here to avoid unnecessary copying
+      gchar* data;
+      gsize length;
+      GError* error = NULL;
+      if(!g_file_get_contents(filepath.c_str(), &data, &length, &error))
       {
-#else
-      if(error.get() != NULL)
-      {
-        const Glib::Exception& ex = *error.get();
-#endif
         App_Glom* pApp = get_application();
         if(pApp)
-          Frame_Glom::show_ok_dialog(_("Image loading failed"), _("The image file could not be opened:\n") + ex.what(), *pApp, Gtk::MESSAGE_ERROR);
+          Frame_Glom::show_ok_dialog(_("Image loading failed"), _("The image file could not be opened:\n") + Glib::ustring(error->message), *pApp, Gtk::MESSAGE_ERROR);
+        g_error_free(error);
+      }
+      else
+      {
+        GdaBinary* bin = g_new(GdaBinary, 1);
+        bin->data = reinterpret_cast<guchar*>(data);
+        bin->binary_length = length;
+
+        m_original_data = Gnome::Gda::Value();
+        m_original_data.Glib::ValueBase::init(GDA_TYPE_BINARY);
+        gda_value_take_binary(m_original_data.gobj(), bin);
+
+        m_pixbuf_original = Utils::get_pixbuf_for_gda_value(m_original_data);
+        if(m_pixbuf_original)
+        {
+          m_image.set(m_pixbuf_original); //Load the image.
+          scale();
+          signal_edited().emit();
+        }
+        else
+        {
+	  m_original_data = Gnome::Gda::Value();
+          g_warning("ImageGlom::on_menupopup_activate_select_file(): file load failed.");
+        }
       }
     }
   }
@@ -494,6 +501,9 @@ void ImageGlom::on_clipboard_received_image(const Glib::RefPtr<Gdk::Pixbuf>& pix
 
   if(pixbuf)
   {
+    // Clear original data of previous image
+    m_original_data = Gnome::Gda::Value();
+
     m_pixbuf_original = pixbuf;
 
     m_image.set(m_pixbuf_original); //Load the image.
diff --git a/glom/utility_widgets/imageglom.h b/glom/utility_widgets/imageglom.h
index 7cf59c9..3367570 100644
--- a/glom/utility_widgets/imageglom.h
+++ b/glom/utility_widgets/imageglom.h
@@ -81,6 +81,7 @@ private:
  
   Gtk::Image m_image;
   Gtk::Frame m_frame;
+  mutable Gnome::Gda::Value m_original_data; // Original file data (mutable so that we can create it in get_value() if it does not exist yet)
   Glib::RefPtr<Gdk::Pixbuf> m_pixbuf_original; //Only stored temporarily, because it could be big.
   Glib::RefPtr<Gdk::Pixbuf> m_pixbuf_clipboard; //When copy is used, store it here until it is pasted.
 
diff --git a/glom/utils_ui.cc b/glom/utils_ui.cc
index 3329809..ad9bca6 100644
--- a/glom/utils_ui.cc
+++ b/glom/utils_ui.cc
@@ -259,7 +259,7 @@ Glib::RefPtr<Gdk::Pixbuf> Utils::get_pixbuf_for_gda_value(const Gnome::Gda::Valu
       Glib::RefPtr<Gdk::PixbufLoader> refPixbufLoader;
       try
       {
-        refPixbufLoader = Gdk::PixbufLoader::create(GLOM_IMAGE_FORMAT);
+        refPixbufLoader = Gdk::PixbufLoader::create();
       }
       catch(const Gdk::PixbufError& ex)
       {
diff --git a/glom/xsl_utils.cc b/glom/xsl_utils.cc
index fcc8fa3..c202fdc 100644
--- a/glom/xsl_utils.cc
+++ b/glom/xsl_utils.cc
@@ -30,6 +30,7 @@
 //#include <libexslt/exslt.h> //For exsltRegisterAll().
 #include <giomm.h>
 #include <glibmm/i18n.h>
+#include <gtk/gtk.h>
 
 #include <sstream> //For stringstream
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]