Merge lp:~openerp-community/openobject-client/zehk_use-of-xdg-open into lp:openobject-client

[Thierry Treyer]

Add the use of xdg-open, gnome-open and kde-open to the method used to determine which program should be launched to print the data of a Binary fields.

[xrg (xrg)]

On Wednesday 28 September 2011, Zehk' wrote:
> Zehk' has proposed merging
> lp:~openerp-community/openobject-client/zehk_use-of-xdg-open into
> lp:openobject-client.
>
> For more details, see:
> https://code.launchpad.net/~openerp-community/openobject-client/zehk_use-of
> -xdg-open/+merge/77333
>
> Add the use of xdg-open, gnome-open and kde-open to the method used to
> determine which program should be launched to print the data of a Binary
> fields.

Many thanks!
the previous code has been a "shame to have" ..

--
Say NO to spam and viruses. Stop using Microsoft Windows!

[Naresh(OpenERP) (nch-openerp)]

Hello Zehk,

It seems a good work but I have few doubts... Why do you need to add this extra check for 'posix' and for file types ['xdg-open', 'gnome-open', 'kde-open'] dont we have this check in _findPDFOpener except for the 'kde-open'opener (which can be added there itself) at line 107.

AFAIS the current system do make a check for the applications to be loaded to open the files based on their types or am I missing something ?

Thanks,

review: Needs Information

[lp:~openerp-community/openobject-client/zehk_use-of-xdg-open]

1961. By Thierry Treyer on 2011-09-29

Use of _findInPath

[Thierry Treyer (zehkae)]

> Hello Zehk,
>
> It seems a good work but I have few doubts... Why do you need to add this
> extra check for 'posix' and for file types ['xdg-open', 'gnome-open', 'kde-
> open'] dont we have this check in _findPDFOpener except for the 'kde-
> open'opener (which can be added there itself) at line 107.
>
> AFAIS the current system do make a check for the applications to be loaded to
> open the files based on their types or am I missing something ?
>
>
> Thanks,

Hello,

The problem is when you want to open a document, but don't use OpenOffice (LibreOffice, for instance).
The client is not able to detect LibreOffice, so it won't open your document and will finally crash.
It was tested on Debian 6.0.2, using the packaged version of openerp-client (5.0.12) and using the latest version from the bzr repository.

In addition of using a defined list of application, it add a step to check if it can use 'xdg-open', that will take care of finding which application should be launched, according to the user preferences.

Finally, I've just modified the code to use _findInPath, instead of defining a function that do the same job.

[Naresh(OpenERP) (nch-openerp)]

Hello,

Is doing this not enough ?

nch@Naresh:~/workspace/OpenERP2011/Trunk/client/bin$ bzr diff
=== modified file 'bin/printer/printer.py'
--- bin/printer/printer.py 2011-03-17 12:59:55 +0000
+++ bin/printer/printer.py 2011-10-04 08:57:06 +0000
@@ -104,7 +104,7 @@
         else:
             if options.options['printer.preview']:
                 if not softpath or (softpath and softpath in ['None','none']):
- prog = self._findInPath(['xdg-open', 'gnome-open', 'see'])
+ prog = self._findInPath(['xdg-open', 'gnome-open', 'see','kde-open'])
                     return self._opener(lambda fn: os.execv(prog, (os.path.basename(prog), fn)))
                 else:
                     return self._opener(lambda fn: os.execv(softpath, (os.path.basename(softpath), fn)))

correct me if I am missing something !

Regards,

review: Needs Fixing

[Thierry Treyer (zehkae)]

> Hello,
>
> Is doing this not enough ?
>
> nch@Naresh:~/workspace/OpenERP2011/Trunk/client/bin$ bzr diff
> === modified file 'bin/printer/printer.py'
> --- bin/printer/printer.py 2011-03-17 12:59:55 +0000
> +++ bin/printer/printer.py 2011-10-04 08:57:06 +0000
> @@ -104,7 +104,7 @@
> else:
> if options.options['printer.preview']:
> if not softpath or (softpath and softpath in
> ['None','none']):
> - prog = self._findInPath(['xdg-open', 'gnome-open',
> 'see'])
> + prog = self._findInPath(['xdg-open', 'gnome-open', 'see
> ','kde-open'])
> return self._opener(lambda fn: os.execv(prog,
> (os.path.basename(prog), fn)))
> else:
> return self._opener(lambda fn: os.execv(softpath,
> (os.path.basename(softpath), fn)))
>
> correct me if I am missing something !
>
> Regards,

The problem is not with the PDF opener, it is in general.

If he can't find the right application by using your list (defined for instance in line 119), it will search for another one who will open the document according with the user preferences.

[Thierry Treyer (zehkae)]

xdg-open can open any type of file, not only PDF. That's why we can use it to open any file.

[lp:~openerp-community/openobject-client/zehk_use-of-xdg-open]

1962. By Thierry Treyer on 2011-10-13

Rewrite the Printer module

1963. By Thierry Treyer on 2011-10-13

Fix the Windows part

1964. By Thierry Treyer on 2011-10-13

Merge from main branche

[Thierry Treyer (zehkae)]

Hello,

I rewrote the Printer module.
Now, it use a default opener, according to the OS (start on Windows, open on Mac, xdg-open on Linux,…).

It has been tested on Debian 6.0.3, Windows XP and on Mac OS X (with the OpenERP Client 5).

[Thierry Treyer (zehkae)]

Hello everyone,

I didn't receive any comment since a while, so I come back.
I took into account your remarks and modify the code acordingly.

I would like to have some feedbacks.

Thanks

[Naresh(OpenERP) (nch-openerp)]

Hello Thierry,

Thanks for your valuable contribution ! Just a few comments ! There's always a big risk associated when you make a big change, risk like loosing essential features, regressions etc. Its should be tested and well executed keeping in mind the previous functionalities offered. By loosing essential features I meant that Currently OpenERP allows you to define your own extensions from the extension manager options from the toolbar(GTK client) options/extension manager for eg: I would like to open/print *XYZ* extension files with firefox. Then OpenERP gives higher priority to the extensions defined by the user if found use them else go for the other openers according to extension type. So with your merge proposal this features gets lost.
secondly: OpenERP allows users to define their softpath(usually for pdf) and softpath_html(usuallyfor html types) i.e
If you want to use another PDF viewer or if you do not want to use the first one the OpenERP will find for you, you have to edit the OpenERP configuration file, normally located in ~/.openerprc and in the [printer] section edit the softpath and softpath_html parameter. This feature is also lost with your merge proposal.
Thirdly: can you please refactor your code please like remove the unused imports, unnecessary dict(self.openers) can be just a list as they all gives the same self._findOpener function.

please before making such changes please have a deep look at the existing system working and the features it provides. For eg: if you remove a line then first just think why was the line introduced. do I cover the intension in my new proposal etc.

Once again thanks for your work and patience ! Hope to see the improved merge soon.

Regards,

review: Needs Fixing

[Etienne Bagnoud (etienne-bagnoud)]

Hi,

I'm interested in sorting out this bug and have followed this thread. I think the rewrite of the module might be too much, but the original proposition was pretty good as it didn't introduce any regression (not modifying too much code) and worked.
xdg-open is part of XdgUtils which make it simpler for an application to use the correct program to open any given file or URL (http://portland.freedesktop.org/wiki/). OpenERP GTK Client uses xdg-open only for pdf file and an intern list for others application. When it doesn't find the right application it just crash. The first proposition keeps the original solution while adding the correct way of opening a file under operating systems that respect freedesktop and user's preferences.

Of course a rewrite is needed for this module, using a generic file opener (xdg-open) and not for others files shows a lack of understanding of what does xdg-open.

Maybe the first proposition should be merged in stable branch as first easy step : it correct a bug under Debian (and maybe others distributions) and does not introduce any regression and the last proposition should be made a basis for a new correct code for opening a file.

My 2 cents.
Etienne.

Unmerged revisions

1964. By Thierry Treyer on 2011-10-13

Merge from main branche

1963. By Thierry Treyer on 2011-10-13

Fix the Windows part

1962. By Thierry Treyer on 2011-10-13

Rewrite the Printer module

1961. By Thierry Treyer on 2011-09-29

Use of _findInPath

1960. By Thierry Treyer on 2011-09-28

Using xdg-open, gnome-open and kde-open to open the file of a Binary field

1959. By Thierry Treyer on 2011-09-28

Merge from main branch

=== modified file 'bin/printer/printer.py'
--- bin/printer/printer.py	2011-03-17 12:59:55 +0000
+++ bin/printer/printer.py	2011-10-13 08:37:34 +0000
@@ -44,144 +44,58 @@
 
     def __init__(self):
         self.openers = {
-            'pdf': self._findPDFOpener,
-            'html': self._findHTMLOpener,
-            'doc': self._findHTMLOpener,
-            'xls': self._findHTMLOpener,
-            'sxw': self._findSXWOpener,
-            'odt': self._findSXWOpener,
-            'tiff': self._findPDFOpener,
+            'pdf': self._findOpener,
+            'html': self._findOpener,
+            'doc': self._findOpener,
+            'xls': self._findOpener,
+            'sxw': self._findOpener,
+            'odt': self._findOpener,
+            'tiff': self._findOpener,
         }
 
     def _findInPath(self, progs):
         lstprogs = progs[:]
-        found = {}
-        path = [dir for dir in os.environ['PATH'].split(':')
+        path = [dir for dir in os.environ['PATH'].split(os.pathsep)
                 if os.path.isdir(dir)]
         for dir in path:
             content = os.listdir(dir)
-            for prog in progs[:]:
+            for prog in lstprogs:
                 if prog in content:
                     return os.path.join(dir, prog)#prog
-
-                    progs.remove(prog)
-                    found[prog] = os.path.join(dir, prog)
-        for prog in lstprogs:
-            if prog in found:
-                return found[prog]
         return ''
 
-    def _findHTMLOpener(self):
-        import webbrowser
-        def opener(fn):
-            webbrowser.open('file://'+fn)
-        return opener
-
     def _opener(self, fnct):
         def opener(fn):
             pid = os.fork()
             if not pid:
-                pid = os.fork()
-                if not pid:
-                    fnct(fn)
-                time.sleep(0.1)
-                sys.exit(0)
+                fnct(fn)
             os.waitpid(pid, 0)
         return opener
+	
+    def _findOpener(self):
+        app_to_run, opener = None, None
 
-    def _findPDFOpener(self):
-        if platform.system() == 'Darwin':
-            return self._opener(lambda fn: os.system('open ' + fn))
-        softpath = options.options['printer.softpath']
         if platform.system() == 'Windows':
-            if options.options['printer.preview']:
-                if not softpath or (softpath and softpath in ['None','none']):
-                    return os.startfile
-                else:
-                    return lambda fn: os.system(softpath + ' ' + fn)
-            else:
-                return print_w32_filename
-        else:
-            if options.options['printer.preview']:
-                if not softpath or (softpath and softpath in ['None','none']):
-                    prog = self._findInPath(['xdg-open', 'gnome-open', 'see'])
-                    return self._opener(lambda fn: os.execv(prog, (os.path.basename(prog), fn)))
-                else:
-                    return self._opener(lambda fn: os.execv(softpath, (os.path.basename(softpath), fn)))
-            else:
-                return print_linux_filename
-
-    def _findSXWOpener(self):
-        if os.name == 'nt':
-            return lambda fn: os.startfile(fn)
-        else:
-            if options.options['printer.softpath_html'] is None:
-                prog = self._findInPath(['ooffice', 'ooffice2', 'openoffice', 'soffice'])
-                def opener(fn):
-                    pid = os.fork()
-                    if not pid:
-                        pid = os.fork()
-                        if not pid:
-                            os.execv(prog, (os.path.basename(prog),fn))
-                        time.sleep(0.1)
-                        sys.exit(0)
-                    os.waitpid(pid, 0)
-                return opener
-            else:
-                def opener(fn):
-                    pid = os.fork()
-                    if not pid:
-                        pid = os.fork()
-                        if not pid:
-                            os.execv(options.options['printer.softpath_html'], (os.path.basename(options.options['printer.softpath_html']),fn))
-                        time.sleep(0.1)
-                        sys.exit(0)
-                    os.waitpid(pid, 0)
-                return opener
+            opener = os.startfile
+        elif platform.system() == 'Darwin':
+            app_to_run = self._findInPath(['open'])
+        else:
+            app_to_run = self._findInPath(['xdg-open', 'gnome-open', 'kde-open'])
+
+        if app_to_run:
+            opener = self._opener(lambda fn: os.execv(app_to_run, (os.path.basename(app_to_run), fn)))
+
+        return opener
+
     def print_file(self, fname, ftype, preview=False):
-        app_to_run = None
-        try:
-            filetypes = eval( options.options['extensions.filetype'] )
-            (app, app_print) = filetypes[ftype]
-            if options.options['printer.preview'] or preview:
-                app_to_run = app
-            else:
-                app_to_run = app_print
-        except:
-            pass
-
-        if app_to_run:
-            def open_file(cmd, filename):
-                cmd = cmd.split()
-                found = False
-                for i, v in enumerate(cmd):
-                    if v == '%s':
-                        cmd[i] = filename
-                        found = True
-                        break
-                if not found:
-                    cmd.append(filename)
-
-                import subprocess
-                subprocess.Popen(cmd)
-            open_file(app_to_run, fname)
-
-        else:
+        opener = self._findOpener()
+        if opener:
             try:
-                finderfunc = self.openers.get(ftype,False)
-                if not finderfunc:
-                    if sys.platform in ['win32', 'nt']:
-                        os.startfile(fname)
-                    else:
-                        finderfunc = self.openers['html']
-                        opener = finderfunc()
-                        opener(fname)
-                else:
-                    opener = finderfunc()
-                    opener(fname)
-                    gc.collect()
-            except Exception,e:
+                opener(fname)
+            except:
                 raise Exception(_('Unable to handle %s filetype') % ftype)
+        else:
+            raise Exception(_('Unable to handle %s filetype') % ftype)
 
 printer = Printer()