Most look okay, but some seem a little out of place, like they may be accidentally reverting or changing things you've already changed, or something like that. This is supposed to be essentially just changes for Phoenix and Py3 compatibility.
Metallicow, if you are able to split the AGW changes out into a separate PR that would probably make things easier. Also there are a few other notes I've made in the PR about incomplete or unnecessary changes.
Also, in the future please make smaller PRs with changesets that are grouped and/or focused more logically. For example, things like "Update six module", "Change from old pen styles to wx.PENSTYLE_* names", "Fix bare except clauses for Py3" would be logical groupings and something that I could bang out reviews, tests and commits for several PRs in an evening. But making thousands of only semi-related changes and dumping it all as a single PR makes the review process like trying to drink from a dozen fire hoses. It's 3am and I've been working on this for 6+ hours tonight and am nowhere near being ready to commit the changes.
Sorry about it becoming so big. As I was new to git at the time.
This case in particular I guess would be an exception as big library changes like this don’t happen very often.
After this is all sorted out properly, you won’t probably ever see a commit list that big from me ever again, unless maybe I’m around for the PY4 Phoenix4000 port.
Note to self: More logical branch naming(grouped) in the future. A bit more grouping detail than just library/demo. Ex: AGW/Pubsub/Ext Packages should be a group in itself.
@ Andrea, please review the AGW changes in the library-branch/squash-library-branch. Whatever Robin is pointing to as a Pull REQz.
I have done work on the demos in demo branch also to get them working, that might be of interest after the library stuff has been reviewed/processed.
All the changes done in the library-branch where mostly done for getting the demos to run on PY2/PY3.
···
On Friday, January 10, 2014 5:04:54 AM UTC-6, Robin Dunn wrote:
Andrea, could you please review the AGW changes in PR #26 for me?
Most look okay, but some seem a little out of place, like they may be
accidentally reverting or changing things you’ve already changed, or
something like that. This is supposed to be essentially just changes
for Phoenix and Py3 compatibility.
Metallicow, if you are able to split the AGW changes out into a separate
PR that would probably make things easier. Also there are a few other
notes I’ve made in the PR about incomplete or unnecessary changes.
Also, in the future please make smaller PRs with changesets that are
grouped and/or focused more logically. For example, things like “Update
six module”, “Change from old pen styles to wx.PENSTYLE_* names”, “Fix
bare except clauses for Py3” would be logical groupings and something
that I could bang out reviews, tests and commits for several PRs in an
evening. But making thousands of only semi-related changes and dumping
it all as a single PR makes the review process like trying to drink from
a dozen fire hoses. It’s 3am and I’ve been working on this for 6+ hours
tonight and am nowhere near being ready to commit the changes.
Most look okay, but some seem a little out of place, like they may be accidentally reverting or changing things you’ve already changed, or something like that. This is supposed to be essentially just changes for Phoenix and Py3 compatibility.
I’ve taken a look at the PR, and to me it seems OK with the following exception:
In “cubecolourdialog.py”, a new class called “InvisibleFrame” has been introduced with the purpose of grabbing a colour from the screen. Unfortunately, it depends on PIL or pyscreenshot, which may not be available at runtime. It would be better if the dependency could be made optional.
I haven’t yet made use of the power of pull requests, but I guess it’s time for me to navigate through the git world a bit…
Metallicow, if you are able to split the AGW changes out into a separate PR that would probably make things easier. Also there are a few other notes I’ve made in the PR about incomplete or unnecessary changes.
Also, in the future please make smaller PRs with changesets that are grouped and/or focused more logically. For example, things like “Update six module”, “Change from old pen styles to wx.PENSTYLE_* names”, “Fix bare except clauses for Py3” would be logical groupings and something that I could bang out reviews, tests and commits for several PRs in an evening. But making thousands of only semi-related changes and dumping it all as a single PR makes the review process like trying to drink from a dozen fire hoses. It’s 3am and I’ve been working on this for 6+ hours tonight and am nowhere near being ready to commit the changes.
Last I checked the wxPy way of getting a screenie only worked right with 2.8
2.9 had wreaked the image/had artifacts. Not sure if that got fixed with 3.0…
I was going to get around to adding the imports at the top of the file and make a Global If PILIMPORTED/PYSCREENSHOTIMPORTED, so to check.
Guess that should make its way in.
Actually if the wxPython way of grabbing a screenshot is fixed, then the extra dependencies could be removed or default to one or another depending on the wxversion.
Would this be a better way to handle this… overall?
···
On Sunday, February 9, 2014 5:43:02 AM UTC-6, Infinity77 wrote:
Hi Robin and All,
On 10 January 2014 12:04, Robin Dunn wrote:
Andrea, could you please review the AGW changes in PR #26 for me?
Most look okay, but some seem a little out of place, like they may be accidentally reverting or changing things you’ve already changed, or something like that. This is supposed to be essentially just changes for Phoenix and Py3 compatibility.
I’ve taken a look at the PR, and to me it seems OK with the following exception:
In “cubecolourdialog.py”, a new class called “InvisibleFrame” has been introduced with the purpose of grabbing a colour from the screen. Unfortunately, it depends on PIL or pyscreenshot, which may not be available at runtime. It would be better if the dependency could be made optional.
I haven’t yet made use of the power of pull requests, but I guess it’s time for me to navigate through the git world a bit…
Metallicow, if you are able to split the AGW changes out into a separate PR that would probably make things easier. Also there are a few other notes I’ve made in the PR about incomplete or unnecessary changes.
Also, in the future please make smaller PRs with changesets that are grouped and/or focused more logically. For example, things like “Update six module”, “Change from old pen styles to wx.PENSTYLE_* names”, “Fix bare except clauses for Py3” would be logical groupings and something that I could bang out reviews, tests and commits for several PRs in an evening. But making thousands of only semi-related changes and dumping it all as a single PR makes the review process like trying to drink from a dozen fire hoses. It’s 3am and I’ve been working on this for 6+ hours tonight and am nowhere near being ready to commit the changes.
OK I seemed to find out about wx.ScreenDC.GetPixel(). It seems to work right currently. Removed PIL/PySS deps.
@ Robin: PR#30 is ready to go now. Tested and Run/works fine. I didn’t test saving the saving of screenies yet, so beware… there might still be the ugly artifacts problem(maybe that was only with saving?)
Anywho. Whatever. Better than nothing.
Also there is an issue that crept in elsewhere since 2.9.5 with the current 3.0.1 something todo with GCDC???
···
Running Python 2
Traceback (most recent call last):
File “C:\Python27\lib\site-packages\wx-3.0.1-phoenix\wx\lib\agw\cubecolourdialog.py”, line 2428, in OnPaint
self.DrawAlphaShading(mem_dc, rect)
File “C:\Python27\lib\site-packages\wx-3.0.1-phoenix\wx\lib\agw\cubecolourdialog.py”, line 2449, in DrawAlphaShading
gcdc = wx.GCDC(dc)
wx._core.wxAssertionError: C++ assertion “m_handle” failed at …..\src\msw\dib.cpp(580) in wxDIB::CreatePalette(): wxDIB::CreatePalette(): invalid object
Traceback (most recent call last):
File “C:\Python27\lib\site-packages\wx-3.0.1-phoenix\wx\lib\agw\cubecolourdialog.py”, line 2563, in OnPaint
gcdc = wx.GCDC(mem_dc)
wx._core.wxAssertionError: C++ assertion “m_handle” failed at …..\src\msw\dib.cpp(580) in wxDIB::CreatePalette(): wxDIB::CreatePalette(): invalid object
Traceback (most recent call last):
File “C:\Python27\lib\site-packages\wx-3.0.1-phoenix\wx\lib\agw\cubecolourdialog.py”, line 2563, in OnPaint
gcdc = wx.GCDC(mem_dc)
wx._core.wxAssertionError: C++ assertion “m_handle” failed at …..\src\msw\dib.cpp(580) in wxDIB::CreatePalette(): wxDIB::CreatePalette(): invalid object