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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
_______________________________________________
LibreOffice mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
