Re: [Gimp-developer] refactor palette loading code
Thanks for letting me know. wt On Sep 17, 2013 4:44 PM, Michael Henning dra...@darkrefraction.com wrote: wt: Your new patch looks good, so I committed it. Thanks! As a side note, I modified the message slightly. We normally include the general area of the commit (often the top level directory) as a prefix to the commit message. Also, I removed the signed off by line because we don't really use that. The short paragraph probably wasn't necessary in this case (we know what refactoring means), but I left that in anyway. :) commit 198f2514ab03cd77c769b0cea9678fa0deba4f6e Author: Warren Turkal w...@penguintechs.org Date: Sat Sep 14 23:46:28 2013 -0700 app: Refactor palette loaders. I specifically moved the file opening/closing logic to the common code. This makes the code easier to understand for me since there is less duplication. In fact, this commit removes more lines than it adds. After I committed it, Mitch asked me to change the function names from your patch on irc. You can see that commit here: https://git.gnome.org/browse/gimp/commit/?id=d02dd9f0da778640a0a8a82420ee22f9a6efc943 On Mon, Sep 16, 2013 at 4:56 AM, Michael Schumacher schum...@gmx.de wrote: Gesendet: Montag, 16. September 2013 um 09:34 Uhr Von: Warren Turkal w...@penguintechs.org I am willing to do whatever is needed to contribute. However, it would be nice if the mailing list wouldn't block patches. Please keep in mind that those file would be sent out to every subscriber of this mailing list, even those who would not want to receive them. Now imagine people being subscribed to 10 mailing lists. Would you want to receive all patches and bundles from all the projects you're subscribed to? The preferred way right now is to open bug reports in Bugzilla and attach your patches there. Has anyone taken a look at maybe using gerrit? It's actually a pretty reasonable way to handle code changes when using git. It has a pretty nice code review workflow. Projects like Android and Libreoffice use it. As an example, here's a link https://gerrit.libreoffice.org/#/q/status:open,n,zto the Libreoffice instance. Might be worthwhile to discuss that with GNOME; it's their repository we're using after all. They could be interesed in this especially for their GNOME Love bugs, see https://wiki.gnome.org/GnomeLove -- Regards, Michael ___ gimp-developer-list mailing list List address:gimp-developer-list@gnome.org List membership: https://mail.gnome.org/mailman/listinfo/gimp-developer-list ___ gimp-developer-list mailing list List address:gimp-developer-list@gnome.org List membership: https://mail.gnome.org/mailman/listinfo/gimp-developer-list ___ gimp-developer-list mailing list List address:gimp-developer-list@gnome.org List membership: https://mail.gnome.org/mailman/listinfo/gimp-developer-list
Re: [Gimp-developer] refactor palette loading code
wt: Your new patch looks good, so I committed it. Thanks! As a side note, I modified the message slightly. We normally include the general area of the commit (often the top level directory) as a prefix to the commit message. Also, I removed the signed off by line because we don't really use that. The short paragraph probably wasn't necessary in this case (we know what refactoring means), but I left that in anyway. :) commit 198f2514ab03cd77c769b0cea9678fa0deba4f6e Author: Warren Turkal w...@penguintechs.org Date: Sat Sep 14 23:46:28 2013 -0700 app: Refactor palette loaders. I specifically moved the file opening/closing logic to the common code. This makes the code easier to understand for me since there is less duplication. In fact, this commit removes more lines than it adds. After I committed it, Mitch asked me to change the function names from your patch on irc. You can see that commit here: https://git.gnome.org/browse/gimp/commit/?id=d02dd9f0da778640a0a8a82420ee22f9a6efc943 On Mon, Sep 16, 2013 at 4:56 AM, Michael Schumacher schum...@gmx.de wrote: Gesendet: Montag, 16. September 2013 um 09:34 Uhr Von: Warren Turkal w...@penguintechs.org I am willing to do whatever is needed to contribute. However, it would be nice if the mailing list wouldn't block patches. Please keep in mind that those file would be sent out to every subscriber of this mailing list, even those who would not want to receive them. Now imagine people being subscribed to 10 mailing lists. Would you want to receive all patches and bundles from all the projects you're subscribed to? The preferred way right now is to open bug reports in Bugzilla and attach your patches there. Has anyone taken a look at maybe using gerrit? It's actually a pretty reasonable way to handle code changes when using git. It has a pretty nice code review workflow. Projects like Android and Libreoffice use it. As an example, here's a link https://gerrit.libreoffice.org/#/q/status:open,n,zto the Libreoffice instance. Might be worthwhile to discuss that with GNOME; it's their repository we're using after all. They could be interesed in this especially for their GNOME Love bugs, see https://wiki.gnome.org/GnomeLove -- Regards, Michael ___ gimp-developer-list mailing list List address:gimp-developer-list@gnome.org List membership: https://mail.gnome.org/mailman/listinfo/gimp-developer-list ___ gimp-developer-list mailing list List address:gimp-developer-list@gnome.org List membership: https://mail.gnome.org/mailman/listinfo/gimp-developer-list
Re: [Gimp-developer] refactor palette loading code
I am willing to do whatever is needed to contribute. However, it would be nice if the mailing list wouldn't block patches. Has anyone taken a look at maybe using gerrit? It's actually a pretty reasonable way to handle code changes when using git. It has a pretty nice code review workflow. Projects like Android and Libreoffice use it. As an example, here's a link https://gerrit.libreoffice.org/#/q/status:open,n,zto the Libreoffice instance. wt On Sun, Sep 15, 2013 at 2:22 AM, Jehan Pagès jehan.marmott...@gmail.comwrote: Hi, On Sun, Sep 15, 2013 at 8:32 PM, Warren Turkal w...@penguintechs.org wrote: On Tue, Sep 10, 2013 at 6:54 PM, Jehan Pagès jehan.marmott...@gmail.com wrote: On Wed, Sep 11, 2013 at 1:10 PM, Michael Henning dra...@darkrefraction.com wrote: As a side note for the future, the fastest way to get a patch reviewed is normally if you post it to a pastebin and bother people on irc. For my own, I would prefer a git format-patch like this, but on a feature request/bug report on bugzilla. That's easy to patch a branch and to remove after. And also we keep track of any discussion or updated patch about a feature/fix. For instance go find this email thread in one year in the mailing history. Even for small refactorings like this one? I would certainly understand that for a feature add or a major refactor, but it seems like a lot of overhead for a pretty small refactor like this one. However, I am willing to do whatever you folks want since I just wanna help the project. However, please keep in mind that I have very little time to commit to this kind of work. Well there are no strict rules, I guess, likely because the team is small. I guess the real rule is to do so that others are not annoyed by the form your patch (so that they will actually check it, and not delay it to forever). Which means that if the other developers are ok with a git bundle for instance (I did not even know what it is, I had to look it up), or by adding your repo as a remote, well that's all good. :-) I am myself flexible and adapt to various team logics. But by experience, I know some others of the core GIMP team want git format-patch. When I made my first patches (I am myself likely the most recent of the core developers), I also set up a public git repo for others to fetch. Well I was told my patches would more likely be reviewed if I provided patch files instead. And now I got used to it, so I work also easily with these. That's not more time-consuming. With a patch formatted with `git format-patch`, you can just git apply it, and done! So easy to review (and also to commit if the patch looks good with with git am, nothing to do). I believe that at the opposite, for small patches, it is much easier to provide patch files than maintain a public repo. For huge features which require many commits over weeks, yeah there probably a public branch is worth it. :-) Jehan P.S. I don't see the patch on that last email. Are you sure you attached it? I see it but I was also a direct recipient. I guess that the list cleans emails out from any attached file. Could we have conditional filters? Like any text file with a .patch or .diff extension should not be filtered out. You should also allow git bundle files, which are a way to pass around git commits. I have attached one to this mail that includes the second iteration of my change. I guess only direct receivers of the email will receive it. wt ___ gimp-developer-list mailing list List address:gimp-developer-list@gnome.org List membership: https://mail.gnome.org/mailman/listinfo/gimp-developer-list
Re: [Gimp-developer] refactor palette loading code
Gesendet: Montag, 16. September 2013 um 09:34 Uhr Von: Warren Turkal w...@penguintechs.org I am willing to do whatever is needed to contribute. However, it would be nice if the mailing list wouldn't block patches. Please keep in mind that those file would be sent out to every subscriber of this mailing list, even those who would not want to receive them. Now imagine people being subscribed to 10 mailing lists. Would you want to receive all patches and bundles from all the projects you're subscribed to? The preferred way right now is to open bug reports in Bugzilla and attach your patches there. Has anyone taken a look at maybe using gerrit? It's actually a pretty reasonable way to handle code changes when using git. It has a pretty nice code review workflow. Projects like Android and Libreoffice use it. As an example, here's a link https://gerrit.libreoffice.org/#/q/status:open,n,zto the Libreoffice instance. Might be worthwhile to discuss that with GNOME; it's their repository we're using after all. They could be interesed in this especially for their GNOME Love bugs, see https://wiki.gnome.org/GnomeLove -- Regards, Michael ___ gimp-developer-list mailing list List address:gimp-developer-list@gnome.org List membership: https://mail.gnome.org/mailman/listinfo/gimp-developer-list
Re: [Gimp-developer] refactor palette loading code
Sorry for taking so long to respond. I don't have a lot of time during normal work days to work on this. :) On Tue, Sep 10, 2013 at 6:10 PM, Michael Henning dra...@darkrefraction.comwrote: I made a few minor nitpicks on your commit on github. If you would fix those points, I'll commit your code to master. Thanks. I think I addressed all your comments. However, I just added a rewind at the end of gimp_palette_load_detect format instead of doing what your comment suggested. PTAL and see if you approve. Here's the linkhttps://github.com/wt/gimp/tree/refactor_palette_loader_try2. I have also attached a patch to this mail. As a side note for the future, the fastest way to get a patch reviewed is normally if you post it to a pastebin and bother people on irc. I am not in a huge hurry, and I haven't had much luck catching folks on IRC when I am on. Is there really any benefit of using pastebin over pushing my branch out so that it can be looked at and fetched? The pastebin method seems like it'd be pretty inconvenient for bigger patches, and it doesn't have any kind of commenting on the patch. Also, which pastebin do you prefer? I will say that I was surprised there wasn't more interest in using git to pass around these patches. I would have expected you folks to acquire the patch from my repository (e.g. fetch my branch from my repo and look at the branch directly). I was somewhat surprised by the request for a patch file. With regard to git, I don't see any merges in the history for the project. Does this project not do git merges? -- drawoc P.S. I don't see the patch on that last email. Are you sure you attached it? It appears to be attached on my end. Does the ML block attachments? FWIW, I also attached the new diff to this email. It indeed does appear to be attached at this point. wt ___ gimp-developer-list mailing list List address:gimp-developer-list@gnome.org List membership: https://mail.gnome.org/mailman/listinfo/gimp-developer-list
Re: [Gimp-developer] refactor palette loading code
On Tue, Sep 10, 2013 at 6:54 PM, Jehan Pagès jehan.marmott...@gmail.comwrote: On Wed, Sep 11, 2013 at 1:10 PM, Michael Henning dra...@darkrefraction.com wrote: As a side note for the future, the fastest way to get a patch reviewed is normally if you post it to a pastebin and bother people on irc. For my own, I would prefer a git format-patch like this, but on a feature request/bug report on bugzilla. That's easy to patch a branch and to remove after. And also we keep track of any discussion or updated patch about a feature/fix. For instance go find this email thread in one year in the mailing history. Even for small refactorings like this one? I would certainly understand that for a feature add or a major refactor, but it seems like a lot of overhead for a pretty small refactor like this one. However, I am willing to do whatever you folks want since I just wanna help the project. However, please keep in mind that I have very little time to commit to this kind of work. P.S. I don't see the patch on that last email. Are you sure you attached it? I see it but I was also a direct recipient. I guess that the list cleans emails out from any attached file. Could we have conditional filters? Like any text file with a .patch or .diff extension should not be filtered out. You should also allow git bundle files, which are a way to pass around git commits. I have attached one to this mail that includes the second iteration of my change. I guess only direct receivers of the email will receive it. wt ___ gimp-developer-list mailing list List address:gimp-developer-list@gnome.org List membership: https://mail.gnome.org/mailman/listinfo/gimp-developer-list
Re: [Gimp-developer] refactor palette loading code
Hi, On Sun, Sep 15, 2013 at 8:32 PM, Warren Turkal w...@penguintechs.org wrote: On Tue, Sep 10, 2013 at 6:54 PM, Jehan Pagès jehan.marmott...@gmail.com wrote: On Wed, Sep 11, 2013 at 1:10 PM, Michael Henning dra...@darkrefraction.com wrote: As a side note for the future, the fastest way to get a patch reviewed is normally if you post it to a pastebin and bother people on irc. For my own, I would prefer a git format-patch like this, but on a feature request/bug report on bugzilla. That's easy to patch a branch and to remove after. And also we keep track of any discussion or updated patch about a feature/fix. For instance go find this email thread in one year in the mailing history. Even for small refactorings like this one? I would certainly understand that for a feature add or a major refactor, but it seems like a lot of overhead for a pretty small refactor like this one. However, I am willing to do whatever you folks want since I just wanna help the project. However, please keep in mind that I have very little time to commit to this kind of work. Well there are no strict rules, I guess, likely because the team is small. I guess the real rule is to do so that others are not annoyed by the form your patch (so that they will actually check it, and not delay it to forever). Which means that if the other developers are ok with a git bundle for instance (I did not even know what it is, I had to look it up), or by adding your repo as a remote, well that's all good. :-) I am myself flexible and adapt to various team logics. But by experience, I know some others of the core GIMP team want git format-patch. When I made my first patches (I am myself likely the most recent of the core developers), I also set up a public git repo for others to fetch. Well I was told my patches would more likely be reviewed if I provided patch files instead. And now I got used to it, so I work also easily with these. That's not more time-consuming. With a patch formatted with `git format-patch`, you can just git apply it, and done! So easy to review (and also to commit if the patch looks good with with git am, nothing to do). I believe that at the opposite, for small patches, it is much easier to provide patch files than maintain a public repo. For huge features which require many commits over weeks, yeah there probably a public branch is worth it. :-) Jehan P.S. I don't see the patch on that last email. Are you sure you attached it? I see it but I was also a direct recipient. I guess that the list cleans emails out from any attached file. Could we have conditional filters? Like any text file with a .patch or .diff extension should not be filtered out. You should also allow git bundle files, which are a way to pass around git commits. I have attached one to this mail that includes the second iteration of my change. I guess only direct receivers of the email will receive it. wt ___ gimp-developer-list mailing list List address:gimp-developer-list@gnome.org List membership: https://mail.gnome.org/mailman/listinfo/gimp-developer-list
Re: [Gimp-developer] refactor palette loading code
Hi, On Wed, Sep 11, 2013 at 1:10 PM, Michael Henning dra...@darkrefraction.com wrote: I made a few minor nitpicks on your commit on github. If you would fix those points, I'll commit your code to master. ok cool, someone reviewed the patch before me. :-D As a side note for the future, the fastest way to get a patch reviewed is normally if you post it to a pastebin and bother people on irc. For my own, I would prefer a git format-patch like this, but on a feature request/bug report on bugzilla. That's easy to patch a branch and to remove after. And also we keep track of any discussion or updated patch about a feature/fix. For instance go find this email thread in one year in the mailing history. -- drawoc P.S. I don't see the patch on that last email. Are you sure you attached it? I see it but I was also a direct recipient. I guess that the list cleans emails out from any attached file. Could we have conditional filters? Like any text file with a .patch or .diff extension should not be filtered out. Jehan On Mon, Sep 9, 2013 at 3:27 AM, Warren Turkal w...@penguintechs.org wrote: Attached is the patch. wt On Sun, Sep 8, 2013 at 12:13 PM, Warren Turkal w...@penguintechs.org wrote: Hey, I will get the format-patch as soon as I can. However, if you want to do it the git way and get it sooner, you can always add my remote to your git with the following command: git remote add wt g...@github.com:wt/gimp.git Note that wt can be whatever alias you want, but my commands below assume you use wt as the remote alias. That url for the repo is available on the main page for the repo on that website after a single click. To find it from the commit page that I sent earlier (this link herehttps://github.com/wt/gimp/commit/d1e8c4c8543b18c6f5d95f6ab6b3bbbf8f80778b), look near the top left. You can see it if you look toward the top left. You'll see something like PUBLIC wt/gimp. If you click on the gimp part, it will take you the repo main page (herehttps://github.com/wt/gimp). Look on the right side of the page. You can get https, ssh, and a few other types of urls for the repo. Once you have my remote, you can fetch my repo objects with this commands: git fetch wt Then, you can do all the git operations you want. For example, here's how to get a log: git log wt/refactor_palette_loader If you want to merge in my commit, do the following while on your local working (maybe master) branch: git merge wt/refactor_palette_loader And if you find that you don't want my remote in your repo anymore, you can remove it. Make sure you don't have any branches tracking mine. One way deal with branches tracking mine is to just delete them. Then execute this command: git remote remove wt After doing that, you can also do the following if you to get rid of commits that were only in my repo: git prune wt On Sun, Sep 8, 2013 at 5:33 AM, Jehan Pagès jehan.marmott...@gmail.comwrote: Hi, I searched a little in this web UI, but couldn't find: is there a way to generate a proper patch from this? Otherwise, could you generate one with git format-patch origin/master, run on your locale branch? Thanks. Jehan On Mon, Sep 9, 2013 at 12:20 AM, Warren Turkal w...@penguintechs.org wrote: BTW, I tested this with gpl, act, aco, pal, and css palette files. I couldn't find RIFF file. If anyone has one, would you mind emailing it to me so that I can try loading it? Thanks, wt On Sun, Sep 8, 2013 at 5:06 AM, Warren Turkal w...@penguintechs.org wrote: Hi again, Here's a link https://github.com/wt/gimp/commit/d1e8c4c8543b18c6f5d95f6ab6b3bbbf8f80778bto a commit containing a refactor of the palette loading code. I have moved the file open/close logic to common code. This change actually removes more code than it adds. Here's a interesting diffstat (without whitespace only changes): $ git diff --stat --color -w origin/master app/core/gimp.c | 4 +- app/core/gimppalette-import.c | 37 +-- app/core/gimppalette-load.c | 148 - app/core/gimppalette-load.h | 12 +++- 4 files changed, 85 insertions(+), 116 deletions(-) Any chance this could be pulled into the master? Do y'all have any other thoughts on this? Thanks, wt ___ gimp-developer-list mailing list List address:gimp-developer-list@gnome.org List membership: https://mail.gnome.org/mailman/listinfo/gimp-developer-list ___ gimp-developer-list mailing list List address:gimp-developer-list@gnome.org List membership: https://mail.gnome.org/mailman/listinfo/gimp-developer-list ___ gimp-developer-list mailing list List address:gimp-developer-list@gnome.org List membership: https://mail.gnome.org/mailman/listinfo/gimp-developer-list
[Gimp-developer] refactor palette loading code
Hi again, Here's a linkhttps://github.com/wt/gimp/commit/d1e8c4c8543b18c6f5d95f6ab6b3bbbf8f80778bto a commit containing a refactor of the palette loading code. I have moved the file open/close logic to common code. This change actually removes more code than it adds. Here's a interesting diffstat (without whitespace only changes): $ git diff --stat --color -w origin/master app/core/gimp.c | 4 +- app/core/gimppalette-import.c | 37 +-- app/core/gimppalette-load.c | 148 - app/core/gimppalette-load.h | 12 +++- 4 files changed, 85 insertions(+), 116 deletions(-) Any chance this could be pulled into the master? Do y'all have any other thoughts on this? Thanks, wt ___ gimp-developer-list mailing list List address:gimp-developer-list@gnome.org List membership: https://mail.gnome.org/mailman/listinfo/gimp-developer-list
Re: [Gimp-developer] refactor palette loading code
Hey, I will get the format-patch as soon as I can. However, if you want to do it the git way and get it sooner, you can always add my remote to your git with the following command: git remote add wt g...@github.com:wt/gimp.git Note that wt can be whatever alias you want, but my commands below assume you use wt as the remote alias. That url for the repo is available on the main page for the repo on that website after a single click. To find it from the commit page that I sent earlier (this link herehttps://github.com/wt/gimp/commit/d1e8c4c8543b18c6f5d95f6ab6b3bbbf8f80778b), look near the top left. You can see it if you look toward the top left. You'll see something like PUBLIC wt/gimp. If you click on the gimp part, it will take you the repo main page (here https://github.com/wt/gimp). Look on the right side of the page. You can get https, ssh, and a few other types of urls for the repo. Once you have my remote, you can fetch my repo objects with this commands: git fetch wt Then, you can do all the git operations you want. For example, here's how to get a log: git log wt/refactor_palette_loader If you want to merge in my commit, do the following while on your local working (maybe master) branch: git merge wt/refactor_palette_loader And if you find that you don't want my remote in your repo anymore, you can remove it. Make sure you don't have any branches tracking mine. One way deal with branches tracking mine is to just delete them. Then execute this command: git remote remove wt After doing that, you can also do the following if you to get rid of commits that were only in my repo: git prune wt On Sun, Sep 8, 2013 at 5:33 AM, Jehan Pagès jehan.marmott...@gmail.comwrote: Hi, I searched a little in this web UI, but couldn't find: is there a way to generate a proper patch from this? Otherwise, could you generate one with git format-patch origin/master, run on your locale branch? Thanks. Jehan On Mon, Sep 9, 2013 at 12:20 AM, Warren Turkal w...@penguintechs.org wrote: BTW, I tested this with gpl, act, aco, pal, and css palette files. I couldn't find RIFF file. If anyone has one, would you mind emailing it to me so that I can try loading it? Thanks, wt On Sun, Sep 8, 2013 at 5:06 AM, Warren Turkal w...@penguintechs.org wrote: Hi again, Here's a link https://github.com/wt/gimp/commit/d1e8c4c8543b18c6f5d95f6ab6b3bbbf8f80778bto a commit containing a refactor of the palette loading code. I have moved the file open/close logic to common code. This change actually removes more code than it adds. Here's a interesting diffstat (without whitespace only changes): $ git diff --stat --color -w origin/master app/core/gimp.c | 4 +- app/core/gimppalette-import.c | 37 +-- app/core/gimppalette-load.c | 148 - app/core/gimppalette-load.h | 12 +++- 4 files changed, 85 insertions(+), 116 deletions(-) Any chance this could be pulled into the master? Do y'all have any other thoughts on this? Thanks, wt ___ gimp-developer-list mailing list List address:gimp-developer-list@gnome.org List membership: https://mail.gnome.org/mailman/listinfo/gimp-developer-list ___ gimp-developer-list mailing list List address:gimp-developer-list@gnome.org List membership: https://mail.gnome.org/mailman/listinfo/gimp-developer-list
