Andreas Vox wrote:
> Hi!
>
> I resumed my last year's project to generate bitmap files for Docbook.
> As a first measure I refactored some code from PreviewLoader I want
> to use into its own class, SnippetConversion.
>
> I would appreciate if someone could review this code and tell me how
> to improve it. The code works for me, somewhat. I got a SIGSEGV in
> ForkedcallsController::handleCompletedProcesses() once, but the
> MacOS stacktrace didn't help me to locate the bug. Couldn't reproduce
> it yet, either.
>
> Ciao
> /Andreas
Buona sera, Andreas.
First impressions are great. All those '-' lines in PreviewLoader must be a
good thing. However, before I try and understand what you've done, I want
to tell you of something that the preview code MUST do and currently
doesn't.
It doesn't tell RenderPreview (insets/render_preview.[Ch]) when something
has gone wrong. All the user sees is "Preview Loading" or some such. The
signals are received (by Impl::finishedGenerating and thereafter by the
GraphicsLoader that actually loads the bitmapp file into memory) but
there's no transfer of info back.
I'd really, really love it if you could take on this task as well. I can
answer questions but finding time to write code...
Ok, back to your work.
The diff file looks great. But it's hard to see in detail what's happening.
Could you post the PreviewLoader.C file (yeah, I'm lazy).
SnippetConversion.h: never, ever polute the global namespace. Stuff like
namespace support = lyx::support;
using std::ostream;
typedef list<string> SnippetList;
is just baaaddd. Move the SnippetList typedef inside of SnippetConversion
class. Use the fully qualified names for the rest and then introduce the
shortcuts into the .C file.
There's some whitespace weirdness going on. Use tabs not spaces.
I think you should take the opportunity to add an explanatory comment to
each one of the SnippetConversion member functions. Simple or as
complicated as necessary.
Make pid_t pid private.
If you need an accessor, write
pid_t pid() const { return pid_; }
Ie, the accessor returns a copy of the store which cannot be modified by
the outside world.
It's a LyX convention to add a trailing underscore to all member variables.
So, pid_. Etc.
SnippetConversion.C: more whitespace weirdness.
class IncrementedFileName {
return_type operator()(string const &);
};
Can't this member function be const? It doesn't appear to alter any class
data.
But basically, this file is fine. It's just pulling stuff out of
PreviewLoader.C, no?
Can you outline what you plan to do from here on in?
--
Angus
