Re: Review Request 110327: KMessageWidget: Remove decoration icon
On May 7, 2013, 12:50 p.m., Dominik Haumann wrote: The patch itself is fine and most likely does not introduce regressions in terms of misbehavior. Still, is never showing an icon the way to go? Another way to work around this by default would be an additional function called KMessageWidget::setShowIcon(bool). In fact, I've recently been thinking that being able to set custom icons also may be a good idea, along with setting a custom color for the message widget. Maybe one could ex extend MessageType, i.e.: setMessageType(Custom). Along with setIcon() and setPalette() or similar. Then, the developer would have more control over the colors showing up in the Kate views. A disadvantage of this is, however, that this leads to inconsistent ui's. It this is needed, this patch works against this, though. Aurélien Gâteau wrote: It could be nice to be able to use a custom icon which would be adapted to the context of the message. But I think the widget still works without it for now, and getting rid of it has its advantages (fixing confusion, saving space). An alternative to this fix would be to replace the icon with something less button-like. The only icon I think would work here is the dialog-warning one, which is already used with KMessageWidget::Warning type. Any other idea? Exposing access to the palette would indeed be dangerous for consistency. Can you explain in which situation you would want control over the colors? Thomas Lübking wrote: What about making the icon a watermark? Dominik Haumann wrote: Using icons as watermark was already discussed years ago. I didn't read the entire thread, but on the following link you can find a url to an image showing a watermark icon in the background. The thread is very long, and it the end nothing came out of it apparently: http://lists.kde.org/?l=kde-core-develm=120204190217471w=2 Dominik Haumann wrote: Exposing access to the palette would indeed be dangerous for consistency. Can you explain in which situation you would want control over the colors? Not really: We just had once someone on kwrite-devel (iirc) complaining about the colors. But if you ask me, the colors are fine for the use cases right now. Aurélien Gâteau wrote: I don't like watermarks, they look too noisy. Having thought about this patch more, I would like to keep the ability to set the icon. What about removing all icons by default but adding an icon property? This would fix the confusion for KMessageWidget::Error and still saves space while giving the ability to set icons more adapted to the widget message when there is a need for it. I think we also need a neutral message type that uses the window/button background color (as a gradient, of course) as widget background for displaying things that aren't really an information but rather progress such as the Kate document is still loading message. And for Kate I could think of many cases where, instead of using the generic warning icon or no icon at all, using a more specific one could improve first-sight-recognizability such as 'object-locked' for when the file has been opened read-only, or 'character-set' when there's encoding issues. - Kai Uwe --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110327/#review32194 --- On May 6, 2013, 8:59 p.m., Aurélien Gâteau wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110327/ --- (Updated May 6, 2013, 8:59 p.m.) Review request for Dolphin, Kate and kdelibs. Description --- This avoids confusion between the decoration icon and the close button, especially when type is KMessageWidget::Error. This happens for example with Dolphin when an error happens while trying to connect to an non available host. This change also has the nice side-effect of leaving more space for the widget text. Diffs - kdeui/widgets/kmessagewidget.cpp a52316726233a22929ce8ad3aff60b9ccc5f9b85 Diff: http://git.reviewboard.kde.org/r/110327/diff/ Testing --- Tested with kmessagewidgetdemo, Dolphin and Kate. Thanks, Aurélien Gâteau
kde review kartesio
Hello,I have been working on Kartesio, a program for calculating best fit curves with experimental points. I think it is ready to be moved in the KDE Edu main repo now, so I'm asking your approval.I followed the guidelines (http://techbase.kde.org/Policies/Application_Lifecycle) and Kartesio is actually in KDE review:https://projects.kde.org/projects/kdereview/kartesioFor any question, ask me. Luca Tringali
Re: Review Request 110327: KMessageWidget: Remove decoration icon
On May 7, 2013, 12:50 p.m., Dominik Haumann wrote: The patch itself is fine and most likely does not introduce regressions in terms of misbehavior. Still, is never showing an icon the way to go? Another way to work around this by default would be an additional function called KMessageWidget::setShowIcon(bool). In fact, I've recently been thinking that being able to set custom icons also may be a good idea, along with setting a custom color for the message widget. Maybe one could ex extend MessageType, i.e.: setMessageType(Custom). Along with setIcon() and setPalette() or similar. Then, the developer would have more control over the colors showing up in the Kate views. A disadvantage of this is, however, that this leads to inconsistent ui's. It this is needed, this patch works against this, though. Aurélien Gâteau wrote: It could be nice to be able to use a custom icon which would be adapted to the context of the message. But I think the widget still works without it for now, and getting rid of it has its advantages (fixing confusion, saving space). An alternative to this fix would be to replace the icon with something less button-like. The only icon I think would work here is the dialog-warning one, which is already used with KMessageWidget::Warning type. Any other idea? Exposing access to the palette would indeed be dangerous for consistency. Can you explain in which situation you would want control over the colors? Thomas Lübking wrote: What about making the icon a watermark? Dominik Haumann wrote: Using icons as watermark was already discussed years ago. I didn't read the entire thread, but on the following link you can find a url to an image showing a watermark icon in the background. The thread is very long, and it the end nothing came out of it apparently: http://lists.kde.org/?l=kde-core-develm=120204190217471w=2 Dominik Haumann wrote: Exposing access to the palette would indeed be dangerous for consistency. Can you explain in which situation you would want control over the colors? Not really: We just had once someone on kwrite-devel (iirc) complaining about the colors. But if you ask me, the colors are fine for the use cases right now. Aurélien Gâteau wrote: I don't like watermarks, they look too noisy. Having thought about this patch more, I would like to keep the ability to set the icon. What about removing all icons by default but adding an icon property? This would fix the confusion for KMessageWidget::Error and still saves space while giving the ability to set icons more adapted to the widget message when there is a need for it. Kai Uwe Broulik wrote: I think we also need a neutral message type that uses the window/button background color (as a gradient, of course) as widget background for displaying things that aren't really an information but rather progress such as the Kate document is still loading message. And for Kate I could think of many cases where, instead of using the generic warning icon or no icon at all, using a more specific one could improve first-sight-recognizability such as 'object-locked' for when the file has been opened read-only, or 'character-set' when there's encoding issues. as a gradient, of course http://api.kde.org/4.x-api/kdelibs-apidocs/kdeui/html/classKStyle.html#a679a9290cff89d0f2e330cd448216d2d Reg. the icon not prominent enough that's easily solved by, as Kai suggested, using a sensible icon in the first place (eg. one that does not look like a close button, to start with) and then let it occupy the entire left or right half of the widget, resp. cap that at (dpi adjusted) 128px. Then *partially* overlay it with a translucent bg colored rect and put the text in there. The imporant aspect of all those actions should be to make it look like not-a-button. If you can't imagine what i've in mind, i'll sketch a mock this evening. - Thomas --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110327/#review32194 --- On May 6, 2013, 8:59 p.m., Aurélien Gâteau wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110327/ --- (Updated May 6, 2013, 8:59 p.m.) Review request for Dolphin, Kate and kdelibs. Description --- This avoids confusion between the decoration icon and the close button, especially when type is KMessageWidget::Error. This happens for example with Dolphin when an error happens while trying to connect to an non available host. This change also has the nice side-effect of leaving more space for the
Re: Review Request 110327: KMessageWidget: Remove decoration icon
On May 7, 2013, 8:50 a.m., Dominik Haumann wrote: The patch itself is fine and most likely does not introduce regressions in terms of misbehavior. Still, is never showing an icon the way to go? Another way to work around this by default would be an additional function called KMessageWidget::setShowIcon(bool). In fact, I've recently been thinking that being able to set custom icons also may be a good idea, along with setting a custom color for the message widget. Maybe one could ex extend MessageType, i.e.: setMessageType(Custom). Along with setIcon() and setPalette() or similar. Then, the developer would have more control over the colors showing up in the Kate views. A disadvantage of this is, however, that this leads to inconsistent ui's. It this is needed, this patch works against this, though. Aurélien Gâteau wrote: It could be nice to be able to use a custom icon which would be adapted to the context of the message. But I think the widget still works without it for now, and getting rid of it has its advantages (fixing confusion, saving space). An alternative to this fix would be to replace the icon with something less button-like. The only icon I think would work here is the dialog-warning one, which is already used with KMessageWidget::Warning type. Any other idea? Exposing access to the palette would indeed be dangerous for consistency. Can you explain in which situation you would want control over the colors? Thomas Lübking wrote: What about making the icon a watermark? Dominik Haumann wrote: Using icons as watermark was already discussed years ago. I didn't read the entire thread, but on the following link you can find a url to an image showing a watermark icon in the background. The thread is very long, and it the end nothing came out of it apparently: http://lists.kde.org/?l=kde-core-develm=120204190217471w=2 Dominik Haumann wrote: Exposing access to the palette would indeed be dangerous for consistency. Can you explain in which situation you would want control over the colors? Not really: We just had once someone on kwrite-devel (iirc) complaining about the colors. But if you ask me, the colors are fine for the use cases right now. Aurélien Gâteau wrote: I don't like watermarks, they look too noisy. Having thought about this patch more, I would like to keep the ability to set the icon. What about removing all icons by default but adding an icon property? This would fix the confusion for KMessageWidget::Error and still saves space while giving the ability to set icons more adapted to the widget message when there is a need for it. Kai Uwe Broulik wrote: I think we also need a neutral message type that uses the window/button background color (as a gradient, of course) as widget background for displaying things that aren't really an information but rather progress such as the Kate document is still loading message. And for Kate I could think of many cases where, instead of using the generic warning icon or no icon at all, using a more specific one could improve first-sight-recognizability such as 'object-locked' for when the file has been opened read-only, or 'character-set' when there's encoding issues. Thomas Lübking wrote: as a gradient, of course http://api.kde.org/4.x-api/kdelibs-apidocs/kdeui/html/classKStyle.html#a679a9290cff89d0f2e330cd448216d2d Reg. the icon not prominent enough that's easily solved by, as Kai suggested, using a sensible icon in the first place (eg. one that does not look like a close button, to start with) and then let it occupy the entire left or right half of the widget, resp. cap that at (dpi adjusted) 128px. Then *partially* overlay it with a translucent bg colored rect and put the text in there. The imporant aspect of all those actions should be to make it look like not-a-button. If you can't imagine what i've in mind, i'll sketch a mock this evening. This example may be relevant, even though its using the warning mode: http://wstaw.org/m/2013/03/27/plasma-desktopTm3877.png Perhaps the confusion is due to autoraise being enabled if there is only the close button? Regards, Eli - Eli --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110327/#review32194 --- On May 6, 2013, 4:59 p.m., Aurélien Gâteau wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110327/ --- (Updated May 6, 2013, 4:59 p.m.) Review request for Dolphin, Kate and kdelibs. Description ---
Re: kde review kartesio
Hi, I am wondering what is the user base for this application as it seems quite specialized (I did not build it yet though). Can you tell us more about the potential target? Another question that comes to mind is: can't it be a feature of an existing KDE Edu apps? Best regards, Anne-Marie - Mail original - De: LucaTringali tringalinv...@libero.it À: kde-core-devel@kde.org Envoyé: Jeudi 9 Mai 2013 18:06:16 Objet: kde review kartesio Hello, I have been working on Kartesio, a program for calculating best fit curves with experimental points. I think it is ready to be moved in the KDE Edu main repo now, so I'm asking your approval. I followed the guidelines ( http://techbase.kde.org/Policies/Application_Lifecycle ) and Kartesio is actually in KDE review: https://projects.kde.org/projects/kdereview/kartesio For any question, ask me. Luca Tringali
Re: kde review kartesio
Quite Unlikely ... It's a Solver, to fit curves into points, That's very used in any theorical research, engeniering, math, phisics, etc. 2013/5/10 Anne-Marie Mahfouf annemarie.mahf...@free.fr Hi, I am wondering what is the user base for this application as it seems quite specialized (I did not build it yet though). Can you tell us more about the potential target? Another question that comes to mind is: can't it be a feature of an existing KDE Edu apps? Best regards, Anne-Marie - Mail original - De: LucaTringali tringalinv...@libero.it À: kde-core-devel@kde.org Envoyé: Jeudi 9 Mai 2013 18:06:16 Objet: kde review kartesio Hello, I have been working on Kartesio, a program for calculating best fit curves with experimental points. I think it is ready to be moved in the KDE Edu main repo now, so I'm asking your approval. I followed the guidelines ( http://techbase.kde.org/Policies/Application_Lifecycle ) and Kartesio is actually in KDE review: https://projects.kde.org/projects/kdereview/kartesio For any question, ask me. Luca Tringali
Re: Review Request 110083: Make kdepim libs optional dependency for libkfbapi
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110083/ --- (Updated May 10, 2013, 10:32 a.m.) Review request for kdelibs and KDEPIM-Libraries. Changes --- Make InvalidTimeZone const class member of UserInfo. Description --- kdepim-libs are needed in the facebook library only to return user info as KABC::Addressee and note as KMime::Message, plus events use KCalCore classes. This patch makes dependency on kdepim-libs optional and disables building the event classes completely in case kdepim-libs was not found. Diffs (updated) - CMakeLists.txt 5aefdcf LibKFbAPI-KDEPIM.pc.in PRE-CREATION LibKFbAPI-KDEPIMConfig.cmake.in PRE-CREATION LibKFbAPI.pc.in af537d1 libkfbapi/CMakeLists.txt dac62bc libkfbapi/commentinfo.h e5578c7 libkfbapi/kdepim-utils.h PRE-CREATION libkfbapi/kdepim-utils.cpp PRE-CREATION libkfbapi/likeinfo.h e06052e libkfbapi/noteinfo.h 2492db1 libkfbapi/noteinfo.cpp 154593d libkfbapi/notificationinfo.h a882694 libkfbapi/userinfo.h ac22a7e libkfbapi/userinfo.cpp 26c64da libkfbapi/userinfoparser_p.h 0189a17 Diff: http://git.reviewboard.kde.org/r/110083/diff/ Testing --- Builds correctly here in both cases Thanks, Martin Klapetek
Re: Review Request 110083: Make kdepim libs optional dependency for libkfbapi
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110083/#review32310 --- Ship it! Ship It! - Kevin Krammer On May 10, 2013, 10:32 a.m., Martin Klapetek wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110083/ --- (Updated May 10, 2013, 10:32 a.m.) Review request for kdelibs and KDEPIM-Libraries. Description --- kdepim-libs are needed in the facebook library only to return user info as KABC::Addressee and note as KMime::Message, plus events use KCalCore classes. This patch makes dependency on kdepim-libs optional and disables building the event classes completely in case kdepim-libs was not found. Diffs - CMakeLists.txt 5aefdcf LibKFbAPI-KDEPIM.pc.in PRE-CREATION LibKFbAPI-KDEPIMConfig.cmake.in PRE-CREATION LibKFbAPI.pc.in af537d1 libkfbapi/CMakeLists.txt dac62bc libkfbapi/commentinfo.h e5578c7 libkfbapi/kdepim-utils.h PRE-CREATION libkfbapi/kdepim-utils.cpp PRE-CREATION libkfbapi/likeinfo.h e06052e libkfbapi/noteinfo.h 2492db1 libkfbapi/noteinfo.cpp 154593d libkfbapi/notificationinfo.h a882694 libkfbapi/userinfo.h ac22a7e libkfbapi/userinfo.cpp 26c64da libkfbapi/userinfoparser_p.h 0189a17 Diff: http://git.reviewboard.kde.org/r/110083/diff/ Testing --- Builds correctly here in both cases Thanks, Martin Klapetek
Re: kde review kartesio
Hi, A few primary remarks: - libzorbaneural is needed but my distro does not have anything with neural in it (OpenSuse 12.3) what repo do I need to add in order to get it? The libzorbaneural website should be added to the cmake file so people can find this and packagers can add it to their distros. - I see a screenshot folder and some .pro files that probably are not needed - some doxygen comments for the variables in the .h files would be appreciated, if anyone else wants to fix bugs it'll help a lot. - Kartesio does not build for me, I get /home/kde-devel/kartesio/src/calculations.cpp:278:1: error: control reaches end of non-void function [-Werror=return-type] cc1plus: some warnings being treated as errors - I don't see a Messages.sh file to extract translatable strings. - I am not comfortable with the rm call line 181 in calculations.cpp = you can probably use more Qt classes here and in other parts of this file too. That's only a quick review as I couldn't run the app yet. Tomaz, as for the user base maybe we could start a module for advanced scientific tools? Best regards, Anne-Marie - Mail original - De: Tomaz Canabrava tcanabr...@kde.org À: Anne-Marie Mahfouf annemarie.mahf...@free.fr Cc: LucaTringali tringalinv...@libero.it, kde-core-devel@kde.org Envoyé: Vendredi 10 Mai 2013 12:28:54 Objet: Re: kde review kartesio Quite Unlikely ... It's a Solver, to fit curves into points, That's very used in any theorical research, engeniering, math, phisics, etc. 2013/5/10 Anne-Marie Mahfouf annemarie.mahf...@free.fr Hi, I am wondering what is the user base for this application as it seems quite specialized (I did not build it yet though). Can you tell us more about the potential target? Another question that comes to mind is: can't it be a feature of an existing KDE Edu apps? Best regards, Anne-Marie - Mail original - De: LucaTringali tringalinv...@libero.it À: kde-core-devel@kde.org Envoyé: Jeudi 9 Mai 2013 18:06:16 Objet: kde review kartesio Hello, I have been working on Kartesio, a program for calculating best fit curves with experimental points. I think it is ready to be moved in the KDE Edu main repo now, so I'm asking your approval. I followed the guidelines ( http://techbase.kde.org/Policies/Application_Lifecycle ) and Kartesio is actually in KDE review: https://projects.kde.org/projects/kdereview/kartesio For any question, ask me. Luca Tringali
Re: kde review kartesio
The app sounds awesome. From the application life cycle page you linked: When you have made one of more releases and want to continue to develop it, the term 'playground' does no longer apply to you. That is the right time to move out of here There are no releases on download.kde.org under unstable. Have you made these releases elsewhere? If so can you provide a link. Thanks David Edmundson
Re: kde review kartesio
Annma, I find that proposal *very* good. I'm a bit distant of KDE programming - I know - because my day job is making me work 12h+ creating scientific tools. ( actually - one of the tools that I created here was a... Solver, to fit curves on points... ) Tomaz 2013/5/10 David Edmundson da...@davidedmundson.co.uk The app sounds awesome. From the application life cycle page you linked: When you have made one of more releases and want to continue to develop it, the term 'playground' does no longer apply to you. That is the right time to move out of here There are no releases on download.kde.org under unstable. Have you made these releases elsewhere? If so can you provide a link. Thanks David Edmundson
Re: kde review kartesio
Hey!
A good thing, I think such a tool could be useful to me too (and I
know a lot of other people to whom it might be useful). Here's what I
noticed from a quick look (some has been said already I think):
* You probably shouldn't track the kdev4 file in the repository, same
goes for screenshots
* zorbaneural is a very fancy dependency, it's not even in arch's AUR.
You should put the git URL into the cmake message.
* I put x**2 into the fit box and clicked Fit, and it crashed:
http://paste.kde.org/741026/
* Did you think about laying out the UI around a splitter? On my
screen, the table takes most of the area and the plot is quite small,
and I can't change that...
* It would be useful to be able to import data in some way, e.g. from
a CSV file. I don't see a way to get data into the program except
typing every number into the cells -- or is there another way? If
there is, could it be made more obvious eventually?
* What does the code in calculations.cpp:117 do? It looks quite
curious. Isn't there a more elegant solution (it looks a bit like a
QChar::isLetter() implementation)?
* calculations.cpp:505 and 584 the same code like in 117 again? It's
weird enough to have that stuff once, but copied multiple times is bad
imho ;)
* Your code uses mixed tab- and space indent (sometimes it uses tabs,
sometimes spaces for no apparent reason). Most KDE apps use only
spaces, you might consider if you want to do that too. Sometimes, the
indent is even missing completely; you should indent one level after
each opening curly parenthesis.
* Same goes for the whole formatting of the code, it's pretty
inconsistent. For example, look at the spaces around operators or so.
* Instead of writing to /tmp/kartesiotmp.txt you should probably use
QTempFile. That will also take care of the deleting the temp file when
it gets deallocated so you don't need to exec (scary and
platform-dependent) rm commands.
* calculations.cpp:277 this makes no sense, there's a statement behind
a return
In general, you're mixing a lot of plain C / stdlib stuff into Qt
code. Is there a reason for that? For example, in calculations.cpp:148
you take text from a text field, convert it to a byte array, convert
it to a char* and then pass it to a function. Why not just pass the
QString? You can iterate over a QString like
foreach ( const QChar c, myqstring ) { ... }
or also
for ( int i = 0; i myqstring.size(); i++ ) { ... }
if you like that better, and you can also index it like a char*, as in
mystring[i+1] or so.
Also, nothing in your code is const and everything is public, although
almost everything could be const and private, but I won't get started
on that now ;)
This is not meant as a list of what you must fix, it's just my two cents.
Cheers!
Sven
2013/5/10 Tomaz Canabrava tcanabr...@kde.org:
Annma, I find that proposal *very* good.
I'm a bit distant of KDE programming - I know - because my day job is making
me work 12h+ creating scientific tools.
( actually - one of the tools that I created here was a... Solver, to fit
curves on points... )
Tomaz
2013/5/10 David Edmundson da...@davidedmundson.co.uk
The app sounds awesome.
From the application life cycle page you linked:
When you have made one of more releases and want to continue to develop
it, the term 'playground' does no longer apply to you. That is the right
time to move out of here
There are no releases on download.kde.org under unstable. Have you
made these releases elsewhere? If so can you provide a link.
Thanks
David Edmundson
Re: Re: Re: kde review kartesio
On Fri, May 10, 2013 at 3:38 PM, LucaTringali tringalinv...@libero.it wrote: The fact is that, before releasing the first binary package, I would like to be sure the code respects KDE guidelines. Otherwise, I would need to create a second package just to adjust the code for KDE. Luca Tringali Messaggio originale Da: da...@davidedmundson.co.uk Data: 10/05/2013 15.23 A: LucaTringalitringalinv...@libero.it Ogg: Re: Re: kde review kartesio On Fri, May 10, 2013 at 2:49 PM, LucaTringali tringalinv...@libero.it wrote: Hi, actually I have not prepared any binary package. If you have not reached a point of making a 0.1 release I would say it is not ready to be considered for extragear at this point in time. I have no objections to reviews before a release, that makes a lot of sense :) Hopefully those above help. I merely object to a move to extragear before making any releases, and so do the guidelines, which is how I interpreted your initial email. Regards David
Re: Review Request 110083: Make kdepim libs optional dependency for libkfbapi
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110083/#review32318 --- CMakeLists.txt http://git.reviewboard.kde.org/r/110083/#comment24041 Apparently installing LibKFbAPI-KDEPIMConfig.cmake in $LIB.../cmake/LibKFbAPI makes KDEPIM-Runtime not able to find it. Is there a way or do I need to install it to $LIB.../cmake/LibKFbAPI-KDEPIM/ ? - Martin Klapetek On May 10, 2013, 10:32 a.m., Martin Klapetek wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110083/ --- (Updated May 10, 2013, 10:32 a.m.) Review request for kdelibs and KDEPIM-Libraries. Description --- kdepim-libs are needed in the facebook library only to return user info as KABC::Addressee and note as KMime::Message, plus events use KCalCore classes. This patch makes dependency on kdepim-libs optional and disables building the event classes completely in case kdepim-libs was not found. Diffs - CMakeLists.txt 5aefdcf LibKFbAPI-KDEPIM.pc.in PRE-CREATION LibKFbAPI-KDEPIMConfig.cmake.in PRE-CREATION LibKFbAPI.pc.in af537d1 libkfbapi/CMakeLists.txt dac62bc libkfbapi/commentinfo.h e5578c7 libkfbapi/kdepim-utils.h PRE-CREATION libkfbapi/kdepim-utils.cpp PRE-CREATION libkfbapi/likeinfo.h e06052e libkfbapi/noteinfo.h 2492db1 libkfbapi/noteinfo.cpp 154593d libkfbapi/notificationinfo.h a882694 libkfbapi/userinfo.h ac22a7e libkfbapi/userinfo.cpp 26c64da libkfbapi/userinfoparser_p.h 0189a17 Diff: http://git.reviewboard.kde.org/r/110083/diff/ Testing --- Builds correctly here in both cases Thanks, Martin Klapetek
Re: Re: kde review kartesio
Hi Luca, Yes, the correct way to express a power is ^. So you should write x^2. I figured that after it had crashed ;) There should be a check routine to avoid that a dangerous string like ** is used, and I'm surely integrating this check in the next release. In my opinion you should not release your application with such an obvious crash bug. It's not only this expression, the program crashes whenever there is anything wrong in that text field. That's not something you can defer to the next release. Actually there is not: I'm working on a new window for the next releases: basically, there will be a button over the table, something like Edit datas. This will open a new window in which it will be possible to import/export CSV, sort X axis values, add other rows or deleting some... I don't know if it makes sense, but you should have a look at the calligra sheets part (aka talk to someone who knows about it). Eventually that can be very useful for this purpose (unfortunately I don't know exactly how they work). Could this instruction be shorter and more elagant? Probably. But it works, and actually I think it could stay as it is. You could replace a 2000+ character boolean logic expression which lists all the letters in the alphabet by this: c.isLetter() || QString(+-*/^).contains(c) at least assuming you do the other thing I said and use QString instead of char*. This is a cleanup which is worth doing. I know it, I'll try to make the code more readable, but I do not have so much time so usually I prefer to dedicate my time to new features or corrections instead of making them prettier. Writing correctly formatted code is mostly a matter of setting up your editor correctly. Spend five minutes doing that ;) Greetings, Sven
Re: Review Request 110043: Proposed fix/workaround for legacy encoded filename handling
On May 7, 2013, 10:11 a.m., Róbert Szókovács wrote: The solution is intentionally shy, I really don't want to fan the flames surrounding this issue. I just stumbled upon this location when it can be handled painlessly. Whether or not it should be turned on by default, in my opinion, can be left for distributors. Thomas Lübking wrote: Then it's worthless. When I encounter broken filenames on a rw device, i know it's time for a fix. When I encounter broken filenames in joliet or rockridge (latter usually caused by myself long ago - thank you, wodim...) i know it's time to mount norock/nojoliet. Whether i do that or set a (KDE only affecting) env makes hardly a difference. When my little sister™ encounters broken filenames anywhere, she knows that it's time to call her personal IT (me) with these files won't open! - if she could not call me, she had no access to those files. Period. She won't think to google for kde broken filenames, because she would not think it's a problem with the name - the files have weird names, yes, but essentially they won't open when she clicks them. That this could be due to some restrictions in UTF-8 and QString and other terms she does not know, cannot be an expected consideration. So either this is not a fixworthy issue at all, or it (as OPT-IN) only becomes a way for distro discrimination (works on distro X but not on distro Y) because fact is that the filenames are broken and if we want to assist in that situation, we assist the unskilled *only* and the unskilled simply dont set env vars. If they did, they were also skilled enough for convmv et al. to deal with that issue correctly. IOW *every* distro but Arch/Gentoo/LFS - ie. where you read a wiki for setup - likely would *have* to set this anyway and those have the users to turn it off at will. /2¢ OK, I'm all for making this on by default, but that would be a change from the current situation, when the default is QFile's filename encoding, basen on locale. If this becomes the default, it disrupts those who use a non-UTF8 locale. The current code provides an enviroment variable to force KDE to threat the filenames UTF8, this patch piggybacks that mechanism. Should we check the locale the same way QFile does? - Róbert --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110043/#review32184 --- On May 7, 2013, 4:14 p.m., Róbert Szókovács wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110043/ --- (Updated May 7, 2013, 4:14 p.m.) Review request for kdelibs and Thiago Macieira. Description --- This patch works around the problem of filenames that are not valid UTF8 strings: in KLocalePrivate::initFileNameEncoding() KDE sets the QFile's encoding/decoding function, to to/fromUTF8() in QString, which in turn calls QUtf8's converter function (QUtf8 is not exported to developers, so I had to use an inefficient method, I think it would be better if we could use the state parameter for error detection). I replaced this with the said functions' copy/pasted version and changed it, so when it encounters an invalid UTF8 string, it will encode it byte by byte, mapping the lower 128 their normal unicode place and the upper 128 to U+18000-U+1807F, and of course the decoder reverses it. To make this actually work you have to define the KDE_UTF8_FILENAMES enviroment variable to a specific value (broken_names). To test it, do the following: .kde/env/KDE_UTF8_FILENAMES.sh with this content: export KDE_UTF8_FILENAMES=broken_names logout, login, try dolphin on faulty files. (instead of the usual boxed ? you'll see just boxes) This addresses bug 165044. http://bugs.kde.org/show_bug.cgi?id=165044 Diffs - kdecore/localization/klocale_kde.cpp b010e74 kdecore/localization/klocale_p.h af4a768 Diff: http://git.reviewboard.kde.org/r/110043/diff/ Testing --- Thanks, Róbert Szókovács
Re: Review Request 110043: Proposed fix/workaround for legacy encoded filename handling
On May 7, 2013, 10:11 a.m., Róbert Szókovács wrote: The solution is intentionally shy, I really don't want to fan the flames surrounding this issue. I just stumbled upon this location when it can be handled painlessly. Whether or not it should be turned on by default, in my opinion, can be left for distributors. Thomas Lübking wrote: Then it's worthless. When I encounter broken filenames on a rw device, i know it's time for a fix. When I encounter broken filenames in joliet or rockridge (latter usually caused by myself long ago - thank you, wodim...) i know it's time to mount norock/nojoliet. Whether i do that or set a (KDE only affecting) env makes hardly a difference. When my little sister™ encounters broken filenames anywhere, she knows that it's time to call her personal IT (me) with these files won't open! - if she could not call me, she had no access to those files. Period. She won't think to google for kde broken filenames, because she would not think it's a problem with the name - the files have weird names, yes, but essentially they won't open when she clicks them. That this could be due to some restrictions in UTF-8 and QString and other terms she does not know, cannot be an expected consideration. So either this is not a fixworthy issue at all, or it (as OPT-IN) only becomes a way for distro discrimination (works on distro X but not on distro Y) because fact is that the filenames are broken and if we want to assist in that situation, we assist the unskilled *only* and the unskilled simply dont set env vars. If they did, they were also skilled enough for convmv et al. to deal with that issue correctly. IOW *every* distro but Arch/Gentoo/LFS - ie. where you read a wiki for setup - likely would *have* to set this anyway and those have the users to turn it off at will. /2¢ Róbert Szókovács wrote: OK, I'm all for making this on by default, but that would be a change from the current situation, when the default is QFile's filename encoding, basen on locale. If this becomes the default, it disrupts those who use a non-UTF8 locale. The current code provides an enviroment variable to force KDE to threat the filenames UTF8, this patch piggybacks that mechanism. Should we check the locale the same way QFile does? There should be no regression in regular use on non broken FS names for no-one - not even those using non UTF-8 locales, so yes - testing the locale to dis/enable this sounds reasonable. Is the solution as simple as deactivating it if the tested env is set to anything but non_broken_names? - Thomas --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110043/#review32184 --- On May 7, 2013, 4:14 p.m., Róbert Szókovács wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110043/ --- (Updated May 7, 2013, 4:14 p.m.) Review request for kdelibs and Thiago Macieira. Description --- This patch works around the problem of filenames that are not valid UTF8 strings: in KLocalePrivate::initFileNameEncoding() KDE sets the QFile's encoding/decoding function, to to/fromUTF8() in QString, which in turn calls QUtf8's converter function (QUtf8 is not exported to developers, so I had to use an inefficient method, I think it would be better if we could use the state parameter for error detection). I replaced this with the said functions' copy/pasted version and changed it, so when it encounters an invalid UTF8 string, it will encode it byte by byte, mapping the lower 128 their normal unicode place and the upper 128 to U+18000-U+1807F, and of course the decoder reverses it. To make this actually work you have to define the KDE_UTF8_FILENAMES enviroment variable to a specific value (broken_names). To test it, do the following: .kde/env/KDE_UTF8_FILENAMES.sh with this content: export KDE_UTF8_FILENAMES=broken_names logout, login, try dolphin on faulty files. (instead of the usual boxed ? you'll see just boxes) This addresses bug 165044. http://bugs.kde.org/show_bug.cgi?id=165044 Diffs - kdecore/localization/klocale_kde.cpp b010e74 kdecore/localization/klocale_p.h af4a768 Diff: http://git.reviewboard.kde.org/r/110043/diff/ Testing --- Thanks, Róbert Szókovács
Re: Review Request 110327: KMessageWidget: Remove decoration icon
On May 7, 2013, 12:50 p.m., Dominik Haumann wrote: The patch itself is fine and most likely does not introduce regressions in terms of misbehavior. Still, is never showing an icon the way to go? Another way to work around this by default would be an additional function called KMessageWidget::setShowIcon(bool). In fact, I've recently been thinking that being able to set custom icons also may be a good idea, along with setting a custom color for the message widget. Maybe one could ex extend MessageType, i.e.: setMessageType(Custom). Along with setIcon() and setPalette() or similar. Then, the developer would have more control over the colors showing up in the Kate views. A disadvantage of this is, however, that this leads to inconsistent ui's. It this is needed, this patch works against this, though. Aurélien Gâteau wrote: It could be nice to be able to use a custom icon which would be adapted to the context of the message. But I think the widget still works without it for now, and getting rid of it has its advantages (fixing confusion, saving space). An alternative to this fix would be to replace the icon with something less button-like. The only icon I think would work here is the dialog-warning one, which is already used with KMessageWidget::Warning type. Any other idea? Exposing access to the palette would indeed be dangerous for consistency. Can you explain in which situation you would want control over the colors? Thomas Lübking wrote: What about making the icon a watermark? Dominik Haumann wrote: Using icons as watermark was already discussed years ago. I didn't read the entire thread, but on the following link you can find a url to an image showing a watermark icon in the background. The thread is very long, and it the end nothing came out of it apparently: http://lists.kde.org/?l=kde-core-develm=120204190217471w=2 Dominik Haumann wrote: Exposing access to the palette would indeed be dangerous for consistency. Can you explain in which situation you would want control over the colors? Not really: We just had once someone on kwrite-devel (iirc) complaining about the colors. But if you ask me, the colors are fine for the use cases right now. Aurélien Gâteau wrote: I don't like watermarks, they look too noisy. Having thought about this patch more, I would like to keep the ability to set the icon. What about removing all icons by default but adding an icon property? This would fix the confusion for KMessageWidget::Error and still saves space while giving the ability to set icons more adapted to the widget message when there is a need for it. Kai Uwe Broulik wrote: I think we also need a neutral message type that uses the window/button background color (as a gradient, of course) as widget background for displaying things that aren't really an information but rather progress such as the Kate document is still loading message. And for Kate I could think of many cases where, instead of using the generic warning icon or no icon at all, using a more specific one could improve first-sight-recognizability such as 'object-locked' for when the file has been opened read-only, or 'character-set' when there's encoding issues. Thomas Lübking wrote: as a gradient, of course http://api.kde.org/4.x-api/kdelibs-apidocs/kdeui/html/classKStyle.html#a679a9290cff89d0f2e330cd448216d2d Reg. the icon not prominent enough that's easily solved by, as Kai suggested, using a sensible icon in the first place (eg. one that does not look like a close button, to start with) and then let it occupy the entire left or right half of the widget, resp. cap that at (dpi adjusted) 128px. Then *partially* overlay it with a translucent bg colored rect and put the text in there. The imporant aspect of all those actions should be to make it look like not-a-button. If you can't imagine what i've in mind, i'll sketch a mock this evening. Eli MacKenzie wrote: This example may be relevant, even though its using the warning mode: http://wstaw.org/m/2013/03/27/plasma-desktopTm3877.png Perhaps the confusion is due to autoraise being enabled if there is only the close button? Regards, Eli And for Kate I could think of many cases where, instead of using the generic warning icon or no icon at all, using a more specific one could improve first-sight-recognizability such as 'object-locked' for when the file has been opened read-only, or 'character-set' when there's encoding issues. This makes a lot of sense to me. Aurélien, what's your opinion on this? Personally, I was never much troubled by the icon itself, but I think it could be useful to hide it, and customize it. - Dominik --- This is an
