Created attachment 161845 [details] [review]
Patch for adding scan capability.

Working on I have created this patch that work for me on Ubuntu karmic. There is necessary to have well configured sane for this run.

I have added a new button one in the toolbar and his callback definition on widgetPresenter.py and implementation of this callback on studioBuilder.py. I don't know if there is the best way so let me know if not.

This patch isn't completed, there are some missing how:

- typical dialog "wait..." when scanning

- scanner selection dialog

have you some suggestions?

Cheers.

Hi David. I'm not the ocrfeeder maintainer, so take the following for what it might be worth:

I don't see a corresponding menu item; just the toolbar button. Though I do see that you've modified menubar_callback_dict which makes me think you might have intended to add a menu item(??), which would be great because:

* Since many users who are blind will prefer menus first and only fall back to exploring toolbars when expected functionality is missing from the menus, having a corresponding menu item is desired.

* Having a corresponding menu item would also bring you into compliance with the GNOME HIG [1], in particular this bit:

    "All functions that appear on your toolbars must also accessible
     via the main menu bar, either directly (i.e. an equivalent menu
     item) or indirectly (e.g. in the Options ▸ Settings dialog)."

* Associating a keyboard shortcut for this functionality so that the menu (and toolbar) could be bypassed would enable the user to be more efficient.

[1] http://library.gnome.org/devel/hig-book/stable/toolbars-appearance.html

Aside from the above, using Lucid, I got a deprecation warning:

/usr/local/lib/python2.6/dist-packages/studio/studioBuilder.py:175: DeprecationWarning: catching of string exceptions is deprecated
  except _sane.error, v:

Hi Joanmarie,

I totally agree.

Soon I'm going to add a new patch with your suggestions.

Review of attachment 161845 [details] [review]:

Hi,

I second Joanmarie's opinion.

I'm rejecting this patch since it needs some things to be better worked.
See the comments to the patch below.

Cheers,

::: studio/studioBuilder.py
@@ +39,3 @@
 
+import sane
+import _sane

If you need this I think you should rather use sane._sane instead of importing that symbol.

@@ +40,3 @@
+import sane
+import _sane
+import threading

Why importing threading if you're not using it?

@@ +45,1 @@
 gtk.gdk.threads_init()

I've just pushed a patch that removes this line. It's here from the beginning by mistake.

@@ +45,3 @@
 gtk.gdk.threads_init()
+
+def raise_error(parent, message, title = _('Canceled Operation')):

If you want to add this function, please add it to the util/lib.py module.

@@ +46,3 @@
+
+def raise_error(parent, message, title = _('Canceled Operation')):
+    error_dlg = gtk.MessageDialog(parent = parent,

In case you didn't notice, most lines of OCRFeeder's code use the convention of naming variables with a clear name, so, no abbreviations like dlg, it should be error_dialog.

Also, the name of the method, raise_error is wrong. You're showing an error dialog, not raising an error. raise_error makes one think of raising an exception.

@@ +51,3 @@
+        message_format = title)
+    error_dlg.format_secondary_text(message)
+    if error_dlg.run() == gtk.RESPONSE_CLOSE:

If it doesn't give a choice to the user, that is, the user can only close it from the window manager or the close button, why comparing the answer?

It should just run the dialog and destroy it in the next line.

@@ +58,3 @@
     EXPORT_FORMATS = ['HTML', 'ODT']
     
+

Please send atomic patches, this patch has many of these whitespace changes. I know OCRFeeder has a lot of trailing spaces that shouldn't be there (I used an editor that caused these without me noticing) but any change to the spaces should be for the whole file and done in a sole commit.

@@ +133,2 @@
     def run(self):
+        gtk.gdk.threads_enter()

I don't think this is necessary here. Actually it should not be needed at all in your patch because you're not using threads.

@@ +167,3 @@
         self.source_images_controler.exportPagesToOdt(self.source_images_selector.getPixbufsSorted())
+
+    gtk.gdk.threads_enter()

You're calling this in the base scope of the class?
This should wrap calls of methods that you want too be handled only in a thread at a time, not the definition of them.

@@ +172,3 @@
+            sane.init()
+            s = sane.open(sane.get_devices()[0][0])
+            image = s.scan()

What lines are you trying to catch an unexpected error? Each of them should throw specific errors or none, so it would be better not to join these in a single try.

@@ +173,3 @@
+            s = sane.open(sane.get_devices()[0][0])
+            image = s.scan()
+        except _sane.error, v:

What does "v" stands for?
Like I wrote before, I like more explanatory names in Python (I can give you reasons for this: no type declarations, etc.) so please don't use "e" when you mean "exception" and certainly don't use "v" or "var" or "variable" :)

@@ +175,3 @@
+        except _sane.error, v:
+            if v == "Error during device I/O":
+                raise_error(self.main_window.window, _('Error during device I/O'))

Maybe sane doesn't have a different class for each error but comparing with the error message might not be the best way.
Also, if the error is not that device I/O, it will return nothing.

I'd prefer to launch a generic scanner error here, just informing the user something went wrong in beginning.

@@ +177,3 @@
+                raise_error(self.main_window.window, _('Error during device I/O'))
+            return False
+        except IndexError, e:

Catching exceptions should be done when you know that an error might be thrown and you cannot treat it otherwise. In other words, it's for unexpected errors that might happen.

The indexes of the devices list can be expected to be wrong and prevented by checking the list so, let's not use a try/except for this.

@@ +181,3 @@
+            return False
+        filename = tempfile.mktemp()
+        image.save(filename, 'TIFF')

Why do you save it in the TIFF format? Why not PNG, for example?

@@ +183,3 @@
+        image.save(filename, 'TIFF')
+        self.__addImagesToReviewer([filename])
+    gtk.gdk.threads_leave()

Same comment as for threads_enter.

Hi Joaquim,

Sure. This patch isn't completed. This was a proof of concept. I understand this patch is not accepted and should not.

I take all this suggestions and I will have in mind for the next patch.

Python code undestand well and almost don't need comments although one would be appreciated.

I will try to do everything on the ocrfeeder's way but I need some clarifications such as widgetModeler.py, why use this?:

<pre>
def performBoxDetection(self, widget):
        image_reviewer = self.__getCurrentReviewer()
        self.performBoxDetectionForReviewer(image_reviewer)
        while gtk.events_pending():
            gtk.main_iteration()
        image_reviewer.addNewEditorsToAllBoxes()
        image_reviewer.performOcrForAllEditors(self.configuration_manager.favorite_engine)
        while gtk.events_pending():
            gtk.main_iteration()

</pre>

Why is needed gtk.events_pending() twice?. It's repeated several times.

Thanks for all.

Cheers.

Have you searched those gtk lines do?
The reason is for the UI not to be freezed for so much time

You must take into account that most of the code was done in a hurry in order for me to deliver my Master Thesis, so there are things which are not really well done but I hope I can improve them, the UI freezing/lack of threads being one of them.


Cheers,

Created attachment 162935 [details] [review]
import from scanner widgetPresenter.py

Created attachment 162936 [details] [review]
import from scanner widgetModeler.py

Created attachment 162937 [details] [review]
import from scanner studioBuilder.py

Created attachment 162938 [details] [review]
util/lib.py patch

Hi,

I have just attached new patches for this feature. Following Joannie's suggestions I have added a menu entry with his accelerator shortcut.

I have a problem, when push the "scanner" button gtk doesn't show the wait dialog. Sometimes it's shown and sometimes it's hidden behind main window. Someone knows why this happen?

Please, let me know if something is wrong or isn't the best way. Thanks

Review of attachment 162938 [details] [review]:

::: util/lib.py
@@ +124,3 @@
+        message_format = title)
+    error_dlg.format_secondary_text(message)
+    if error_dlg.run() == gtk.RESPONSE_CLOSE:

What if the user closes the dialog with the "cross" button?

Review of attachment 162935 [details] [review]:

::: studio/widgetPresenter.py
@@ +95,3 @@
         <toolitem action="ZoomIn"/>
+        <separator/>
+        <toolitem action="ImportFromScanner"/>

The usage of the import from scanner, just like the import from PDF, will not be a common thing as to need to be present in the toolbar. I personally don't like seeing many repeated controls in toolbars, so I would remove this from there (at least until it is clear a common action).

Review of attachment 162936 [details] [review]:

I really doesn't think you have to put this much code in a thread.

Also, calling GTK+ GUIs that might be blocked from the thread doesn't make sense.

Please see my comments.

::: studio/widgetModeler.py
@@ +39,3 @@
+import sane
+import _sane
+from threading import Thread, Lock, Event

You're importing Event and not using it.

@@ +843,3 @@
+    def __init__(self, cbfunc, parent):
+        super(ImportFromScanner_Controller, self).__init__()
+        self.cbfunc = cbfunc

Please don't use names like this. I can conclude it's "callback function" but actually you want just a "show warning function" and a "image scanned function", using this design.

@@ +845,3 @@
+        self.cbfunc = cbfunc
+        self.running = False
+        self.ctrl = True

What does this mean?

@@ +846,3 @@
+        self.running = False
+        self.ctrl = True
+        self.lock = Lock()

Are you sure you need Lock? Why do you need it?

@@ +847,3 @@
+        self.ctrl = True
+        self.lock = Lock()
+        self.lock.acquire()

You're calling the acquire here and within the run as well.

@@ +858,3 @@
+            if not self.ctrl:
+                return
+            try: 

This is a big "try".
"Try"s should wrap code where you know there might be troubles that you can't directly prevent/control.

If there's a problem here, you don't really know what happened, it might even be a typo (SyntaxError) and it will warn the user an error occurred.

I'd prefer the tries only when we cannot prevent the problems.

@@ +876,3 @@
+                    scanner_dev = devices[0][0]
+
+                scan_result = sane.open(scanner_dev).scan()

If there are no devices, you'll still try to scan using a non existing variable.

@@ +889,3 @@
+            try:
+                while gtk.events_pending():
+                   gtk.main_iteration()

There should be no need for this if you didn't use Gtk code inside this thread.

@@ +909,3 @@
+    def __init__(self, parent, text, message):
+        label = gtk.Label(message)
+        super(SimpleMessage, self).__init__(parent = parent, title = text, flags = gtk.WINDOW_TOPLEVEL, buttons = None)

I know Python doesn't require the super to be the first line of the constructors but I think that we should use it this way when we can, things get clear this way.

@@ +925,3 @@
+        super(SimpleMessage, self).hide_all()
+
+    def destroy(self):

Apart from the hide, where you override it to do hide_all, these methods are redundant, you don't need to redefine these methods. They'll be accessed from the parent.

About the hide overriding, I think you should just call hide_all..

@@ +936,3 @@
+        keys = self.devices.keys()
+
+        for element in keys[1:]:

Although this code works it is really hard to understand, what you wanna do here is:


for device in self.devices.values():
    device.set_group(self.devices[keys[0]])

@@ +939,3 @@
+            self.devices[element].set_group(self.devices[keys[0]])
+
+        super(ScannerChooser, self).__init__(parent = parent, title = "Devices selector", flags = gtk.DIALOG_MODAL, buttons = (gtk.STOCK_CANCEL, gtk.RESPONSE_REJECT, gtk.STOCK_OK, gtk.RESPONSE_ACCEPT))

Again, the super should be the first line.

@@ +940,3 @@
+
+        super(ScannerChooser, self).__init__(parent = parent, title = "Devices selector", flags = gtk.DIALOG_MODAL, buttons = (gtk.STOCK_CANCEL, gtk.RESPONSE_REJECT, gtk.STOCK_OK, gtk.RESPONSE_ACCEPT))
+        for dev in self.devices.keys():

Again:

for device in self.devices.values():
    self.vbox.pack_start(device)

Review of attachment 162937 [details] [review]:

Sorry if the comments are not detailed enough but I don't have much time to elaborate.

::: studio/studioBuilder.py
@@ +83,3 @@
                                                                self.configuration_manager)
+
+        self.scanner_controller = ImportFromScanner_Controller(self.__check_scanned, self.main_window.window)

This shouldn't be a class variable, it should be in a way where the scanner control GUI is called in one place without the whole class needing to "know" about it.

@@ +85,3 @@
+        self.scanner_controller = ImportFromScanner_Controller(self.__check_scanned, self.main_window.window)
+        self.scanner_controller.start()
+        self.waiting_message = SimpleMessage(self.main_window.window, "wait", "waiting for scan")

I don't like this. This message should be created and shown when there's the need for it, no lie here, with a constant message and be shown/hidden.

@@ +286,3 @@
         gtk.main_quit()
+
+    def __check_scanned(self, image):

Please don't use the same function for two different purposes. In this case I'd put another argument in the class to be able to show a warning.

Hi,

I took a little to review your patches but I haven't tried it (I have no scanner nearby).


Sorry if some comments don't go to deep or provide alternatives but I've not much time to dedicate to this.

(In reply to comment #12)
> Review of attachment 162938 [details] [review]:
> 
> ::: util/lib.py
> @@ +124,3 @@
> +        message_format = title)
> +    error_dlg.format_secondary_text(message)
> +    if error_dlg.run() == gtk.RESPONSE_CLOSE:
> 
> What if the user closes the dialog with the "cross" button?

The concern I have is that experience has shown that at least in some cases the use of gtk_dialog_run() results in a hang if Orca is running. I'm sure that David tested for that scenario given the project he's working on. Still.... Makes me a tad nervous.... :-/

Hi,

I've attached new patches.

Advice: if you have no scanner you only will see a grayish menu entry. Ensure you scanner run executing xsane before.

Created attachment 164606 [details] [review]
widgetPresenter patch

Created attachment 164607 [details] [review]
widgetModeler patch

Created attachment 164608 [details]
Ignore previous. This is the real widgetModeler patch

Created attachment 164609 [details] [review]
studioBuilder patch

(In reply to comment #18)
> Hi,
> 
> I've attached new patches.
> 
> Advice: if you have no scanner you only will see a grayish menu entry. Ensure
> you scanner run executing xsane before.

Hi David,

I've tried your patch.

I'll write a detailed review, as always but I'd like to let you know that you are manually raising an exception within a try.

Also, the "Wait" dialog won't go away after the scan is done.

Trying to close the dialog using the "cross" button ended up freezing the app and once even froze my whole desktop once.

The widgetModeler patch wasn't added as a patch so I'll do the review as a regular comment.

Review of attachment 164606 [details] [review]:

::: studio/widgetPresenter.py
@@ +164,3 @@
                                   ('RecognizeAreas', None, _('Recognize Selected _Areas'), None, _("Recognize Selected Areas"), menu_items['recognize_areas']),
                                   ('GenerateODT', None, _('_Generate ODT'), None, _("Export to ODT"), tool_items['export_to_odt']),
+                                  ('ImportFromScanner', None, _('_Import Page From Scanner'),'<control><shift>c', _("Import From Scanner"), menu_items['import_from_scanner']),

The I mnemonic is already used for the PDF importation in that menu.
Please remember to review the mnemonics for repetition.

@@ +174,3 @@
+        self.sane_scan_menuentry.set_sensitive(False)
+#       only if on toolbar 
+#       self.sane_scan_menuentry.set_icon_name('scanner')

Try to avoid comments that do not add anything valuable to the code.
We're not adding the Import From Scanner action in the toolbar, so, these comments don't belong to the patch.

@@ +1325,3 @@
+        self.vbox.pack_start(label)
+        self.set_title(text)
+#        self.set_decorated(False)

More unneeded comments.

@@ +1329,3 @@
+        self.resize(200,50)
+        self.set_transient_for(parent)
+        self.connect('delete-event', self.__avoid_close)

Closing the dialog with the close button seemed to raise problems (the app froze and was like that until I killed it), you should check this out.

(In reply to comment #23)
> (In reply to comment #18)
> > Hi,
> > 
> > I've attached new patches.
> > 
> > Advice: if you have no scanner you only will see a grayish menu entry. Ensure
> > you scanner run executing xsane before.
> 
> Hi David,
> 
> I've tried your patch.
> 
> I'll write a detailed review, as always but I'd like to let you know that you
> are manually raising an exception within a try.

Sorry were only for testing delete this line.

> 
> Also, the "Wait" dialog won't go away after the scan is done.
> 
> Trying to close the dialog using the "cross" button ended up freezing the app
> and once even froze my whole desktop once.
> 
> The widgetModeler patch wasn't added as a patch so I'll do the review as a
> regular comment.

I'm going to reattach the patches.

Review of attachment 164609 [details] [review]:

The threads don't seem to be well handled since the UI seems to block still.

If you want me to take a further look at it (try to have it working), let me know.

::: studio/studioBuilder.py
@@ +113,2 @@
         dirs = cli_command_retriever.getParams('--dir')
+

Please do not add line breaks outside your changes' scope.

@@ +210,3 @@
+            else:
+                device = self.devices[0][0]
+    

Trailing spaces.

@@ +211,3 @@
+                device = self.devices[0][0]
+    
+            self.main_window.dialog_scan = widgetPresenter.ScanningWaitDialog(self.main_window.window,

Why do you set the dialog_scan as a variable of the main_window? Why not instantiate it as a local variable, pass it to the self.scan method through the arguments, and use it only here?

@@ +220,3 @@
+                self.main_window.dialog_scan.destroy()
+                return
+# Maybe this will be removed:                

What does this comment mean?
Besides, it has trailing spaces.

@@ +229,3 @@
+            msg.run()
+            msg.destroy()
+            

Trailing spaces.

@@ +234,3 @@
+        timevalue, dev, event = args
+        # this sleep makes unlock, if you comment UI not show wait dialog
+        sleep(timevalue)

This is weird.
If this scan code shouldn't block the UI (that's why you're using a thread) but it seems to be doing so.
I wonder if that's due to the Event handling you're doing. If you're only having one scan thread at a time, why do you need to handle the events like this?

(threads are always a tricky thing so please really let me know if I'm missing things)

@@ +237,3 @@
+        try:
+            self.result = sane.open(dev).scan()
+            raise(Exception('RuntimeError'))

You're raising an exception manually here.
Probably you forgot this from a test.

@@ +253,3 @@
+        else:
+            if self.result and not event.isSet():
+                import tempfile

Please do the importations on the top of the file.
Perhaps the best would be for this scanning logic to be in a util/lib.py function. But anyway, that could be later changed.

@@ +260,3 @@
+                return
+        gtk.gdk.threads_enter()
+        self.main_window.dialog_scan.destroy()

Like I wrote in a previous comment, this seems not to be working, the dialog hangs around forever. Maybe the threads are being blocked and control never reaches this line?

@@ +399,3 @@
+            self.devices = None
+
+    def getdevices(self, *args):

Setting the menu's sensitiveness is making the UI slow (freezes a bit) so I again think the threads are not working as they are supposed to.

@@ +402,3 @@
+        timevalue, event = args
+        # this sleep makes unlock, if you comment UI lock until scanners gotten
+        sleep(timevalue)

Same thought about this sleep as above.

This patch hadn't been added as a "patch" in bugzilla, hence the "manual" review.

diff --git a/studio/widgetModeler.py b/studio/widgetModeler.py
index 0a708c4..e213d0c 100644
--- a/studio/widgetModeler.py
+++ b/studio/widgetModeler.py
@@ -835,3 +835,48 @@ class Editor:
         self.data_box.connect('changed_height', self.__updateEditorHeight)
         self.data_box.connect('changed_image', self.__updateEditorImage)
         self.data_box.connect('changed_type', self.__updateBoxColor)
+
+class ScannerChooserDialog(gtk.Dialog):
+

This dialog should have been done in the widgetPresenter.

+    def __init__(self, parent, devices):
+        super(ScannerChooserDialog, self).__init__(parent = parent,
+                                                   title = _('Devices selector'),
+                                                   flags = gtk.DIALOG_MODAL,
+                                                   buttons = (gtk.STOCK_CANCEL, gtk.RESPONSE_REJECT, gtk.STOCK_OK, gtk.RESPONSE_ACCEPT))
+
+        self.label = gtk.Label(_('Select one of scanners detected:'))
+        self.vbox.pack_start(self.label, padding = 5)
+

AFAIR, the GNOME HIG says 6 pixels for vertical separation (just an FYI, not really important at this point).

+        self.selected_device = None
+        self.devices = {}
+
+        self.__add_devices(devices)
+
+        self.vbox.show_all()
+        

Trailing space.

+        for dev in self.devices.keys():
+            self.devices[dev].connect('toggled', self.__on_select_device)
+# select the first 

Comments should be indented together with their target code. And it has a trailing space.

+        self.devices[keys[0]].emit('toggled')
+
+    def __add_devices(self, devices):
+        for device in devices:
+            self.devices[device] = gtk.RadioButton(label=device)
+
+        keys = self.devices.keys()
+
+

Two line breaks, don't make the line breaks number inconsistent with the rest of the code. (Just an advice)

+        for element in keys[1:]:
+            self.devices[element].set_group(self.devices[keys[0]])
+
+        for dev in self.devices.keys():
+            self.vbox.pack_start(self.devices[dev],
+                                 expand = False,
+                                 fill = False,
+                                 padding = 5)
+
+    def __on_select_device(self, widget):
+        self.selected_device = widget.get_label()
+
+    def get_selected_device(self):
+        return self.selected_device
-- 
1.6.3.3


I couldn't try this code yet (I only have one scanner at the moment) but I'll try it later. The above changes are tiny and not critical so either you do them or I'll do them before applying your patch.


Cheers,

(In reply to comment #23)
> (In reply to comment #18)
> > Hi,
> > 
> > I've attached new patches.
> > 
> > Advice: if you have no scanner you only will see a grayish menu entry. Ensure
> > you scanner run executing xsane before.
> 
> Hi David,
> 
> I've tried your patch.
> 
> I'll write a detailed review, as always but I'd like to let you know that you
> are manually raising an exception within a try.

Sorry were only for testing delete this line.

> 
> Also, the "Wait" dialog won't go away after the scan is done.
> 
> Trying to close the dialog using the "cross" button ended up freezing the app
> and once even froze my whole desktop once.
> 
> The widgetModeler patch wasn't added as a patch so I'll do the review as a
> regular comment.

I'm going to reattach the patches.?%

Created attachment 165092 [details] [review]
widgetPresenter patch

Created attachment 165093 [details] [review]
widgetModeler patch

Created attachment 165094 [details] [review]
studioBuilder patch

I have attached new patches.

I have fixed trailing spaces, commets, and testing traces. I hope this works well now.

I'm having some problems with sane binddings, for some reason threads don't act like threads when calling sane functions which return something, that's sane.get_devices, sane.open and scan. I don't know why UI freezes when doing that if all sane stuffs is done in a thread. Maybe a python-sane problem?

Hi David,

I haven't yet tried your new patches because the main issue wasn't the trailing spaces and forgotten tests' sentences but the threads issues.

I've pushed some changes that introduce an easy way to deal with threads, it makes the PDF importation be done in a different thread.
Hopefully it will easy your job, please take a look at it.

Cheers,

Hey David,

About the "scanner" menu sensitivity, maybe the best is to always have the button enabled and check for the scanners once it's chosen. If then there are no scanner devices available we could show a dialog with this information.

This would avoid the slowness when opening the file menu.

Also, I just checked that The GIMP does things this way.

Best regards,

The "acquire image from scanner" feature is already in master.