Krzysztof,
Please STOP touching the icon code and icons.
You have been ignoring the problems others are seeing, have ignored the information provided to you on them, and have been breaking existing functionality.
It is clear that you don't understand the aspects that are tripping you up (since I don't believe you're being malicious about it), but you are causing serious regressions in behavior. I've also have had to spend a lot of time unbreaking things that I otherwise need to be able to finish some of the new work I had been adding.
Also could you *PLEASE* complete the list of icon renamings that I've asked you for. We need this for compatibility, including needing to use it in some icon.cpp code that you removed yesterday. But the list of name changes is very important to document.
In case you're just stuck, I've gotten a start added under src/widgets/mappings.xml
Please get that updated with all the changes you've made. Thanks.
Jon A. Cruz wrote:
You have been ignoring the problems others are seeing, have ignored the information provided to you on them, and have been breaking existing functionality.
In my testing the changes actually fixed some problems. What I did was to remove a lot of the caching system, and replace it with adding built-in icons (gtk_icon_theme_add_builtin_icon). However, if this caused problems, please revert my changes (I won't be able to do anything today)
Regards, Krzysztof Kosiński
Krzysztof,
In my testing the changes actually fixed some problems. What I did was to remove a lot of the caching system, and replace it with adding built-in icons (gtk_icon_theme_add_builtin_icon).
Purely from your description, this may be a violation of the refactoring freeze. Is there any specific user-visible bug you're fixing or functionality you're adding? If not please revert this for now regardless of whether it was the right thing to do or not, because we agreed to not do any more refactoring until the release.
On Apr 13, 2009, at 8:25 AM, bulia byak wrote:
Krzysztof,
In my testing the changes actually fixed some problems. What I did was to remove a lot of the caching system, and replace it with adding built-in icons (gtk_icon_theme_add_builtin_icon).
Purely from your description, this may be a violation of the refactoring freeze. Is there any specific user-visible bug you're fixing or functionality you're adding? If not please revert this for now regardless of whether it was the right thing to do or not, because we agreed to not do any more refactoring until the release.
I'll take care of this.
It seems clear that on the machine Krzysztof is testing he is not seeing the same problems. So he just doesn't have enough test points up there.
However, I did see at least one user-visible bug fixed by the batch of changes. So it makes sense for me to take care of this, since I have at least one system showing the problem (my OS X laptop) and I can observe the bugs before a fix and observe that after a fix the bugs go away without introducing more.
bulia byak wrote:
Purely from your description, this may be a violation of the refactoring freeze. Is there any specific user-visible bug you're fixing or functionality you're adding? If not please revert this for now regardless of whether it was the right thing to do or not, because we agreed to not do any more refactoring until the release.
Here is some detailed explanation I couldn't write earlier. I'm not surprised by Jon's reaction since I nuked a large portion of the file in question. This wasn't refactoring though. I replaced a lot of rather weird and undocumented caching code (it was trying to add each icon as a stock item, which is not the right thing to do when using named icons) with 1 line adding builtin icons, because I discovered that doing so fixes the icons in the eraser toolbar: They are no longer blurred. There is a weird drawing issue instead where part of the icon is repeated, this may a bug in the drawing function of SPIcon. I also removed a function that was never actually executed to reduce the amount of code that needs to be further scrutinized. I tested this in GDB by setting a breakpoint. I found out why the named icons were not used on Windows: the index.theme file was missing (on Linux it is in /usr/share/icons/hicolor, but on Windows there is no system-wide copy of that file at all). The PNG icons are for use on Windows, because our devlibs do not contain the dependencies of GdkPixbuf's SVG loader, and it still doesn't work when I copy them manually (more information here: http://www.nabble.com/svg-pixbuf-loader-on-Windows-td18293105.html ). 16x16 and 24x24 sizes are sufficient, because our icons are designed to look crisp only at those resolutions, and rendering them from SVG does not give any noticeable visual improvement. So I think I'm slowly making progress towards really finalizing this feature.
One thing that might help you is if you got a virtual machine setup for and running Ubuntu 6.06 LTS (Dapper). That is an older OS that is still supported, so is a valid target platform.
I think dapper has its own version of Inkscape in its repos. Moreover it has GTK 2.8 - too low for current SVN. Testing on Hardy (8.04) might be a good idea though. My current machine is a bit underpowered for work in VM to be productive, but I hope to get a faster one before the 0.47 release
So it makes sense for me to take care of this, since I
have at least one system showing the problem (my OS X laptop) and I can observe the bugs before a fix and observe that after a fix the bugs go away without introducing more.
If you could help me iron out the issues on Mac OS X I would be very grateful, because I have no access to that platform. One more note, Jon: I'm pretty sure the code you added in main.cpp to look for icons in more places is redunant, though it won't cause wrong behavior. Firstly, we need to only add our system-wide icons dir there; user locations are already taken care of by GTK. Secondly, searching in g_get_user_data_dirs()/inkscape/icons isn't good, because paths like ~/.config/inkscape/icons won't exist (our .config dir is in uppercase). Searching in g_get_system_data_dirs()/inkscape/icons seems OK, but I'm not clear about the behavior of this when Inkscape is installed both in /usr/local and /usr, or in an entirely different prefix like /opt.
Regards, Krzysztof Kosiński
On Apr 13, 2009, at 12:53 PM, Krzysztof Kosiński wrote:
Here is some detailed explanation I couldn't write earlier. I'm not surprised by Jon's reaction since I nuked a large portion of the file in question. This wasn't refactoring though. I replaced a lot of rather weird and undocumented caching code (it was trying to add each icon as a stock item, which is not the right thing to do when using named icons) with 1 line adding builtin icons, because I discovered that doing so fixes the icons in the eraser toolbar: They are no longer blurred. There is a weird drawing issue instead where part of the icon is repeated, this may a bug in the drawing function of SPIcon. I also removed a function that was never actually executed to reduce the amount of code that needs to be further scrutinized. I tested this in GDB by setting a breakpoint. I found out why the named icons were not used on Windows: the index.theme file was missing
Here is the big problem
There was code in there for a reason
You did not understand the code
Instead of talking with someone on Jabber you just nuked things you did not understand
Instead of listening to reasons and *testing* things you were asked to just test, you changed code without paying attention.
So you've masked a symptom by layering on workarounds. However you did not actually fix a bug, and introduced several regressions and additional bugs.
Oh, and setting a breakpoint to determine a function is not used DOES NOT WORK. You *MUST* search the codebase for references.
REMEMBER! this is cross platform code that is built against many different versions of many optional libraries. Code inspection is really needed for changes
(BTW, the same problem scenario existed for breaking the whiteboard. You didn't have a test configuration with that code endabled, so you did not notice when you broke the whiteboard).
*Please* actually inspect the code before nuking it... and definitely talk with others if you see something "undocumented" that you want to change. You really need to understand code before changing it carelessly.
Jon A. Cruz wrote:
So you've masked a symptom by layering on workarounds. However you did not actually fix a bug, and introduced several regressions and additional bugs.
That can actually be said of the code I removed. It was a very elaborate wokraround for something that should be fixed properly (by properly, I mean in much fewer lines of code). I did not introduce any workarounds. I have also isolated the tiling bug and will commit a fix in a few hours.
Oh, and setting a breakpoint to determine a function is not used DOES NOT WORK. You *MUST* search the codebase for references.
Except it was a static function
(BTW, the same problem scenario existed for breaking the whiteboard. You didn't have a test configuration with that code endabled, so you did not notice when you broke the whiteboard).
In the bug comment about that breakage on Launchpad I already explained that I didn't break Whiteboard (or Whiteboard dialogs to be precise): it was already broken for a very long time. I modified the build system to get rid of link order issues, and a few left-over files re-entered the build. One of those files even contained a syntax error and nobody noticed that. It also had references to nonexistent object members and methods.
Regards, Krzysztof Kosiński
On Apr 16, 2009, at 10:59 AM, Krzysztof Kosiński wrote:
Oh, and setting a breakpoint to determine a function is not used DOES NOT WORK. You *MUST* search the codebase for references.
Except it was a static function
Again, then that shows that you removed code without understanding the *reason* for it being there.
PLEASE if you don't understand something, just drop in the chat room and ask. Don't just remove it without first determining why it is there and what functionality might be affected.
It may not have manifested with your configuration, but it did manifest for others, and resulted in a few bugs being filed.
On Apr 16, 2009, at 10:59 AM, Krzysztof Kosiński wrote:
Jon A. Cruz wrote:
So you've masked a symptom by layering on workarounds. However you did not actually fix a bug, and introduced several regressions and additional bugs.
That can actually be said of the code I removed. It was a very elaborate wokraround for something that should be fixed properly (by properly, I mean in much fewer lines of code). I did not introduce any workarounds. I have also isolated the tiling bug and will commit a fix in a few hours.
OK. To clarify you did address some issues in the legacy Sodipodi icon parts. That was good. Thanks.
However, you undid three critical pieces of code that are used when icons.svg is involved. Now I know you wanted to remove that, but there are many reasons to keep it. Key among those are that using icons.svg does *not* prevent the use of stand-alone themes that you like, and that it makes for a safety fallback for when something when a theme does not have all of the latest and greatest. Oh, and we have users with custom icons.svg files other than the ones we ship, including some that users have done for accessibility reasons (aka there are people with visual handicaps and custom icons.svg files that *need* them to be able to use Inkscape)
The three key functional code areas removed were:
* Legacy name fallback. Among other things this allows users to have more than a single version of Inkscape installed on one system. Although you might not leverage this, many people (including myself) do. I have two or three different simultaneous installs on each of my two main dev boxes.
* 'smart' size caching. GTK+ themes can change on the fly, including *during* program start up. We have that scenario with our app (see the first sample in my mail "Two icon issues" for a sample). For certain icons and in certain scenarios this manifests as very tricky bugs.
* Background rendering of icons. I was asked a while back to add this feature. You don't miss it when you run with your themes, but when the internal rendering is used it really slows down the UI. It also slows initial appearance of menus the first time they are hit, and thus presents the user with a very negative "sluggish feel" when they first try the UI.
I do understand that the files involved have a lot of messy code that has accumulated over the years. I'm finally at the point where I have time to updated some wiki documentation on this (taxes, work crunch, submitting papers for LinuxCon, coordinating for GSoC and tracking down problems to keep students from being disqualified, etc), so look for updates to appear there. I'll be sure to include some info on how to tell what was new functionality and what is outdated.
Also, it is *very* helpful if you can take the time to stop in the chat room (irc or jabber) and communicate with some of the other devs.
In general there is one bit of advice I can give. If you have someone report a bug, you really need to reproduce it locally before committing code fixes for them. Otherwise it's hard to know you're actually fixing the right issue. I think the blurry icon issue is one such.
On Apr 16, 2009, at 10:59 AM, Krzysztof Kosiński wrote:
In the bug comment about that breakage on Launchpad I already explained that I didn't break Whiteboard (or Whiteboard dialogs to be precise): it was already broken for a very long time. I modified the build system to get rid of link order issues, and a few left-over files re-entered the build. One of those files even contained a syntax error and nobody noticed that. It also had references to nonexistent object members and methods.
Technically that is breaking the build.
You can feel free to do such changes, however in doing so you assume the responsibility of working with others to resolve any issues that arise.
For this specific issue, the bigger problem was that people were affected by this, but initially did not get any response nor support from you on it. Just responding to emails and dropping into the jabber/irc room to talk briefly with those individuals would have gone a long way towards alleviating the problem.
On Apr 13, 2009, at 12:53 PM, Krzysztof Kosiński wrote:
...with 1 line adding builtin icons, because I discovered that doing so fixes the icons in the eraser toolbar: They are no longer blurred.
Ah, but you did not fix the blurring bug that I reported.
What's stranger is that I had not been seeing any blurring of those icons since I fixed things for the "select one" sets of things such as the tweak toolbar. (BTW, that was in a completely different area of code, and thus was not directly related)
The blurring I was seeing and still am seeing manifests differently, and I did send out a sample of what I was seeing.
I think the main takeaway here is that for software in general it is important for an engineer to actually reproduce a problem before fixing so that they can verify that the problem existed and that their code changes did the fixes that were intended.
Jon,
Perhaps it would help to document the issues that you have with the icon changes as bugs. Then we could assign Krzysztof to them in order to track their progress better. I know, for one, there are many intertwined issues that I'm confusing together. Have a bug for each might be better to identify each one.
--Ted
On Apr 16, 2009, at 7:32 PM, Ted Gould wrote:
Jon,
Perhaps it would help to document the issues that you have with the icon changes as bugs. Then we could assign Krzysztof to them in order to track their progress better. I know, for one, there are many intertwined issues that I'm confusing together. Have a bug for each might be better to identify each one.
That's actually what I was planing after the wiki documentation. I know in the past creating and assigning bugs wasn't as productive, but with the extra information put out it might work this time.
I think documenting things on the wiki is really the most important, since understanding what came from sodipodi, what has already been fixed, and what needs to be fixed next will really help anyone moving forward.
I also gleaned from his last mails that he's been having a little problem understanding all that, but given how ancient some of that code is, I am not surprised.
But... in this last round of fixes he did cover some of the ancient Sodipodi mess (the cropping/bleeding was due to that), which was a very good thing. Also I definitely some of his added checks to avoid unneeded internal rendering. So what's left is mainly restoring code that I wrote, but while leaving out some legacy bits. That might be simplest for me to put in, since I do know those bits.
On Thu, 2009-04-16 at 19:45 -0700, Jon A. Cruz wrote:
On Apr 16, 2009, at 7:32 PM, Ted Gould wrote:
Perhaps it would help to document the issues that you have with the icon changes as bugs. Then we could assign Krzysztof to them in order to track their progress better. I know, for one, there are many intertwined issues that I'm confusing together. Have a bug for each might be better to identify each one.
That's actually what I was planing after the wiki documentation. I know in the past creating and assigning bugs wasn't as productive, but with the extra information put out it might work this time.
I think the wiki documentation is good, but with the bugs we can also mark them as required for the 0.47 release. I don't know who's going to be the release wardens yet, but I'd say that it's likely most folks would agree that these need to get milestoned for the release.
--Ted
On Apr 16, 2009, at 7:51 PM, Ted Gould wrote:
On Thu, 2009-04-16 at 19:45 -0700, Jon A. Cruz wrote:
On Apr 16, 2009, at 7:32 PM, Ted Gould wrote:
Perhaps it would help to document the issues that you have with the icon changes as bugs. Then we could assign Krzysztof to them in order to track their progress better. I know, for one, there are many intertwined issues that I'm confusing together. Have a bug for each might be better to identify each one.
That's actually what I was planing after the wiki documentation. I know in the past creating and assigning bugs wasn't as productive, but with the extra information put out it might work this time.
I think the wiki documentation is good, but with the bugs we can also mark them as required for the 0.47 release. I don't know who's going to be the release wardens yet, but I'd say that it's likely most folks would agree that these need to get milestoned for the release.
Very true...
However they're actually being resolved very quickly now, so might be fixed before bug logging gets useful. For example, Krzysztof really did a good fix on that icon cropping thing.
If they're not fixed before long, I'll write up details on each. I'll just need to track their details down carefully (some show up only with external icons and only with older librsvg, some show up only with internally rendered, etc.)
On Apr 16, 2009, at 7:45 PM, Jon A. Cruz wrote:
That's actually what I was planing after the wiki documentation. I know in the past creating and assigning bugs wasn't as productive, but with the extra information put out it might work this time.
I think documenting things on the wiki is really the most important, since understanding what came from sodipodi, what has already been fixed, and what needs to be fixed next will really help anyone moving forward.
OK. The initial part of the info is up
http://wiki.inkscape.org/wiki/index.php/Icons
I didn't quite get to the parts of how the current code works, including all the GTK+ fallbacks, etc. However the baseline history really has a lot of information that's needed to understand things, so I needed to get that complete first.
I'll finish up the rest tomorrow
On Apr 13, 2009, at 1:21 AM, Krzysztof Kosiński wrote:
In my testing the changes actually fixed some problems. What I did was to remove a lot of the caching system, and replace it with adding built-in icons (gtk_icon_theme_add_builtin_icon). However, if this caused problems, please revert my changes (I won't be able to do anything today)
Well, you did remove parts of the caching system, but they were needed parts. There are a few scenarios that expose problems, so one needs to be sure to check all of those edge cases.
One thing that might help you is if you got a virtual machine setup for and running Ubuntu 6.06 LTS (Dapper). That is an older OS that is still supported, so is a valid target platform.
And if anyone out there has a distro they want to be sure is covered, they can add it to the wiki page listing version details: http://wiki.inkscape.org/wiki/index.php/Tracking_Dependencies
participants (4)
-
bulia byak -
Jon A. Cruz -
Krzysztof Kosiński -
Ted Gould