Re: [Libreoffice] [PATCH] Bug 39168

Hello Noel,

Well, I just like more if{ <linefeed><code><linefeed> } form even is 
code is just one line long, but you are right, it still works(and maybe 
also nicer to leave it) without { and } As for the other comments, 
probably you're right that it's not the best solution, I just haven't 
understood fully all the objects, as to be honest because of the lot of 
classes this part of code needs a quite big effort to be modified, as 
you must understand all the objects before,which can be timeconfusing. 
But I absoloutly agree, that this is not the nicest solution, that's why 
I just signed it as purposed and not as final patch. Even because it 
keeps a quite annoying problem, that the overwrite dialog in saving will 
pop up wrong. So I don't either think that this patch can be pushed in 
this form. I just pushed it if someone wants to mind with it. By the way 
if you say I should correct it, and send it again, I'll do provided that 
someone is really going to push it afterwards, I just interrupted this 
job because of the debates about the bug. Some members even doubted if 
it should be fixed, so why should I mind about something needless then? 
Finally I have to mention that I cannot work on it at the moment, as I 
am busy with another bug about Base's querywizard. But as soon as I am 
done I could mind with this issue again taking into the consideration 
your requests.

Gabor

2011. 08. 10. 18:25 keltezéssel, Noel Power írta:
> 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
>
>
>
>
> _______________________________________________
> LibreOffice mailing list
> LibreOffice@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/libreoffice