Re: [Libreoffice] [PATCH] Bug 39168

Hi Jenei
On 08/08/11 11:06, Jenei Gábor wrote:
> Hello,
>
> I would like to inform everyone that I attached my purposed patch that 
> fixes the bug 39168 in our bug report page, since it causes an already 
> mentioned problem in the overwriting dialog's popup I don't consider 
> it as something stable, 
[...]
>  Here I also attach the purposed patch and I am waiting for your reviews.
well I have to admit I am not familiar with this hybrid/pdf thing, not 
sure if it really is a good or bad thing, personally I really don't like 
the double extension thing, but anyway I just have some general comments 
on the patch

sfx2/source/dialog/filedlghelper.cxx
====================================

the first hunk in this patch is confusing, it changes the prevailing style of positioning for '{'&  
'}' if there is no good reason to do that then it is better not to make such a change

the second hunk that changes the logic of FileDialogHelper::GetPath is a little worrying ( 'cause I 
guess this is used by many clients ), why was this necessary and how does it change the existing 
behaviour?

sfx2/source/doc/guisaveas.cxx
=============================

2nd hunk is just whitespace change and again it's quite noisy to review

- in 'sal_Bool ModelData_Impl::OutputFileDialog'

+if(bAddStream==sal_True)
the equality check isn't really necessary&
if((bAddStream)
just reads better for me,

- please don't change the prevailing style for '{' / '}'

- using a map to store and retrieve the desired extension based on the filtername would be neater 
and less cluttered that then the it/then construct used in the patch.
 

what's really uncomfortable is ( from a quick apply&  try of the patch ) is the fact that the file 
name you are saving to is sort-of hijacked under the hood. e.g. the file dialogs indicate one thing ( 
save with.pdf extension ) but do something else ( save with a .odt.pdf extension ).
Using a new filter type would seem a more natural solution e.g. a filter type that you could select 
in the 'save-as' dialog that would save the pdf in the required hybrid format and would indicate 
the ( imo horrible dual extension ) The same filter type would be preselected ( if hybrid was 
selected in the pdf export options dialog ) and shown file dialog raised by the export button. But 
then again I only have a very passing familiarity with stuff, maybe someone else might have more 
constructive advice

Noel


Noel