Hi Sebastien,
Sorry for the delay in answering. Been quite busy, and without a
development environment for MyPaint the last weeks.
I just noticed now that your email did not go to the mailing list, so
I'm leaving it unmodified underneath and just replying on top.
First of all, thanks a lot for your patches! I have applied them all
and pushed to master. I will add some more documentation about the
issues you faced
There seems to be some stale comments/questions left in the demo code?
Fix those and on Linux/Mac it should probably use pkg-config instead
of hardcoding paths to json-c library, we can include it in a
brushlib/examples/demoQt4 directory (not including json-c). I don´t
really agree that multiple files is easier to read for examples, but
you are right: It is the norm for Qt demos.
Regarding the string issue with scons on Windows. Perhaps it stems
from not having a "pkg-config" executable?
If not, try breaking in the Python debugger and find out the string
values in the two lowest levels of the stack.
As mentioned on the mailing list in other threads, the license issue
should be resolved soon. Just missing a response from one person for a
minor contribution.
On 4 February 2013 18:24, Sebastien Leon <sl...@pointcarre.com> wrote:
> Hi Jon,
>
> Thanks for your explanations. Few minutes after reading your email I get
> something usable.
> Now it's just a matter of details...
>
>> What makes you say that these operations are costly?
>
> The fact that it was generating some Tile requests... But now that I
> removed begin/end_atomic callbacks this performance issue does no longer
> exists and it behaves as anyone expects...
>
>> Either don't assign a callback to begin_atomic and end_atomic
> (recommended,
>> as you don't do anything there), or chain up to the parent
>> "mypaint_tiled_surface_begin_atomic() and
> mypaint_tiled_surface_end_atomic()"
>> in your callbacks.
>
> So I removed them and it works fine but Damn ! Some explicit comment
> would have been welcome!
>
>> As for the callbacks, and getting access to s_view. Instead of using the
>> MyPaintTiledSurface struct directly as m_tile, you should create your
> own struct, [...]
>
> Ok. Works fine, of course. As you said, thinking about this API as a C++
> one but written in C...
>
>> This also means that you must pass your surface to stroke_to(), not
> the (private) parent:
>> mypaint_brush_stroke_to((MyPaintSurface *)m_tile, ...)
>
> Sure... Both are binary equivalent and equally ugly as they depend of
> the private struct order.
> (parent must be the first declared)
> But considering that this code is C++ manually written in C, I guess it
> makes sense...
>
>> Yes, that line should not be executed on Windows. Patch welcomed
>
> Just added "if sys.platform != "win32":" before the line. See
> patch_RM.patch attached.
>
> Note that I rarely used Git before (use svn for ages), so if what I send
> to you is wrong, just let me know...
>
>> Hmm. Was that the entire traceback? The string './brushlib/SConscript'
> does
>> not even have 87 characters...
>
> Here is the full output. I guess it is some UTF/Ascii encoding issue.
> But I don't have more clues:
>
> C:\Users\seb\Documents\dev\projects\Git\mypaint>scons
> scons: Reading SConscript files ...
> building for 'python' (use scons python_binary=xxx to change)
> using 'python-config' (use scons python_config=xxx to change)
> UnicodeDecodeError: 'ascii' codec can't decode byte 0x82 in position 87:
> ordinal not in range(128):
> File "C:\Users\seb\Documents\dev\projects\Git\mypaint\SConstruct",
> line 179:
> brushlib = SConscript('./brushlib/SConscript')
> File
> "C:\Windows\Python\Scripts\..\Lib\site-packages\scons-2.2.0\SCons\Script\SConscript.py",
> line 614:
> return method(*args, **kw)
> File
> "C:\Windows\Python\Scripts\..\Lib\site-packages\scons-2.2.0\SCons\Script\SConscript.py",
> line 551:
> return _SConscript(self.fs, *files, **subst_kw)
> File
> "C:\Windows\Python\Scripts\..\Lib\site-packages\scons-2.2.0\SCons\Script\SConscript.py",
> line 260:
> exec _file_ in call_stack[-1].globals
> File
> "C:\Users\seb\Documents\dev\projects\Git\mypaint\brushlib\SConscript",
> line 120:
> env.ParseConfig('pkg-config --cflags --libs %s' % ' '.join(pkg_deps))
> File
> "C:\Windows\Python\Scripts\..\Lib\site-packages\scons-2.2.0\SCons\Environment.py",
> line 1554:
> return function(self, self.backtick(command))
> File
> "C:\Windows\Python\Scripts\..\Lib\site-packages\scons-2.2.0\SCons\Environment.py",
> line 594:
> sys.stderr.write(unicode(err))
>
>> python2 brushlib/generate.py brushlib/mypaint-brush-settings-gen.h
> brushlib/brushsettings-gen.h ?
>
> That should be that. I'll add later it to the Qt project file so it
> won't be required to build mypaint or libmypaint to build the Qt demo
> (scons free :-). Thanks.
>
>> I will gladly accept a patch that makes gettext optional. The ifdef
> should
>> be contained in a utility function though, that just returns the
> original string
>> if not build with gettext.
>
> Hum... mixing preproc ("_" & "N_") & real function may not be a good
> idea. I think it is cleaner to do all this stuff in one step with preproc.
> And if you want to put this stuff in a function, you'll still have to
> use some #ifdef outside to avoid the #include <libintl.h> (unless you
> plan to put it inside the function, which is not great either)
> Have a look to the patch and let we know if you still prefer to move
> this to a function (so I should replace all "_" calls to something like
> "translate()" and do the routing stuff in it) but my modification is
> only 5 lines extra and is not invasive.
>
>> Also, the gettext support should probably be opt-in,
>> so call the define "HAVE_GETTEXT" or similar.
>
> I previously used a NO_GETTEXT flag so you should not modify anything to
> your mypaint config file, only to the demo project... But this is your
> choice to do the reverse ;-)
>
>> Could you provide a patch that shows the required changes? Which version
>> of MSVC are you using?
>
> Please find the patches named "000[1..5]-msvc.patch" (the one with
> HAVE_GETTEXT is 0001-msvc.patch)
>
> I use msvc 2005 & 2010. Note that msvc 2012 for C/C++ can not be
> downloaded freely so I don't think it is an important target. Older msvc
> versions will have plenty of trouble to compile Qt4 code...
>
>> I'd like to keep that style of include. The reason it does not use the
> json/
>> directory is to avoid system-installed headers to be picked up
> automatically, [...]
>
> Understood.
>
>> I hope so too! For committing the demo file to the repository, it
> should be
>> a single file in the brushlib/examples/ directory. And ideally
> provided as a
>> git formatted patch
>
> Hic !? You mean I should merge all C++ headers and cpp files into one ?
> And what's about the Qt project file, the json-c library (for windows
> users), the README...?
>
> The goal of this mini-demo project is to provide to anyone familiar with
> Qt a very simple demonstration of libmypaint. Such self-containing
> directory where the user just type qmake and make is very usual in all
> Qt example & demo. Merging all files into one may ruin the "easy to use
> & understand" goal.
>
> if you prefer, I could adapt the relative path so the demoQt4 could be
> inside "examples" (for now it is at the same level) but I can hardy see
> merging everything into one file inside "examples"...
>
> The latest version (seems bug free) is attached here in demoQt4.zip.
>
> Other point : what's about brushmodes.cpp/.h and
> mypaint-tiled-surface.cpp/.h ?
> Martin said that he was ok to turn them ISC (no longer GPL) but they may
> not be only his work. If all the dev involved in these files are ok, you
> may change the license header...
>
>> If you have the time and interest, you should consider
>> coming to Libre Graphics Meeting:
>
> I will seriously consider it !
>
> Best regards
>
> Sébastien
--
Jon Nordby - www.jonnor.com
_______________________________________________
Mypaint-discuss mailing list
Mypaint-discuss@gna.org
https://mail.gna.org/listinfo/mypaint-discuss
