Re: Minuet (music education software) moved to kdereview
El Monday 25 January 2016, a les 21:47:34, Albert Astals Cid va escriure: > El Sunday 24 January 2016, a les 16:50:18, Andreas Cord-Landwehr va escriure: > > Hi Sandro, it is always great when such a cool application lands in KDE > > Edu. I just made a first and rough (since I do not have all dependencies > > yet to really compile and test it) code review. > > > > Here a some minors I noticed: > > * the application does not link against KCrash (which is needed for > > DrKonqi); also in the main.cpp there should be a call to > > "KCrash::initialize();" * an appdata file is missing > > * in the main CMakeLists.txt, for KF5 there is no minimum version and for > > ECM the minimum version should probably be higher than 1.0.0 > > * it looks strange to me that in minuet/cmake/ there are Config-files for > > the 3rd-party library drumstick. My understanding was that such Config > > files should only be shipped with the respective library (maybe someone > > with a deeper CMake-knowledge can comment?) > > * qml-file-localization: It looks strange to me that you are mixing two > > different localization systems with KI18n (in all C++ and UI files) and > > qsTr in all QML files. Since I am not familiar with qsTr, I cannot really > > comment on how it works and if everything is setup correctly; at least > > Messages.sh do not process qml files and hence do not extract strings > > from them, but that might only be necessary for Ki18n? > > Yeah, you should use ki18n in the qml files too. Ah, it seems Yuri already fixed this. Cheers, Albert > > Cheers, > Albert > > > Other than that, the code quality and licensing information look really > > good. > > > > Cheers, > > Andreas
Re: Minuet (music education software) moved to kdereview
El Sunday 24 January 2016, a les 16:50:18, Andreas Cord-Landwehr va escriure: > * it looks strange to me that in minuet/cmake/ there are Config-files for > the 3rd-party library drumstick. My understanding was that such Config > files should only be shipped with the respective library (maybe someone > with a deeper CMake-knowledge can comment?) If upstream ships a cmake file awesome, but if not then we have to find it having our own cmake file. Cheers, Albert
Re: Minuet (music education software) moved to kdereview
El Sunday 24 January 2016, a les 16:50:18, Andreas Cord-Landwehr va escriure: > Hi Sandro, it is always great when such a cool application lands in KDE Edu. > I just made a first and rough (since I do not have all dependencies yet to > really compile and test it) code review. > > Here a some minors I noticed: > * the application does not link against KCrash (which is needed for > DrKonqi); also in the main.cpp there should be a call to > "KCrash::initialize();" * an appdata file is missing > * in the main CMakeLists.txt, for KF5 there is no minimum version and for > ECM the minimum version should probably be higher than 1.0.0 > * it looks strange to me that in minuet/cmake/ there are Config-files for > the 3rd-party library drumstick. My understanding was that such Config > files should only be shipped with the respective library (maybe someone > with a deeper CMake-knowledge can comment?) > * qml-file-localization: It looks strange to me that you are mixing two > different localization systems with KI18n (in all C++ and UI files) and qsTr > in all QML files. Since I am not familiar with qsTr, I cannot really > comment on how it works and if everything is setup correctly; at least > Messages.sh do not process qml files and hence do not extract strings from > them, but that might only be necessary for Ki18n? Yeah, you should use ki18n in the qml files too. Cheers, Albert > > Other than that, the code quality and licensing information look really > good. > > Cheers, > Andreas
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
> On Jan. 23, 2016, 5:31 p.m., Gregor Mi wrote: > > > If someone has changed the shortcut, they should know what shortcut they > > > set it to, right? So having the tooltip just say "To kill a specific > > > window, press the "Kill Window" shortcut (Ctrl-Alt-Esc by default)" > > > should do the trick, right? > > > > In principle, I agree with you here. :) But... > > > > I have these premises in mind: > > > > 1. There might be a situation where the user can't remember what he has > > done. > > 2. I prefer precise over approximate information if it is reasonable easy > > to get (by fixing this https://git.reviewboard.kde.org/r/122981/ it is now > > easily possible and the patch is already written) > > 3. Somewhere I read that tooltips are to be avoided where possible => this > > patch factors some information out of a tooltip which in itself is > > desirable. > > 4. For the user, the discoverability of the feature after applying this > > patch is better than before (though not perfect, sure). > > Thomas Pfeiffer wrote: > 2. If we can get the current shortcut, why can't we get it for the > tooltip? > 3. Wait, are we talking about different tooltips? I do _not_ mean the one > you get when clicking the ? icon in the window decoration. Those should be > avoided because few people even notice that button. What I mean is the thing > that pops up when hovering over a control. Those are strongly encouraged in > the HIG, in fact they should be used on every control where the function > isn't clear from the label. > 4. Clicking a button just to get explained how to use a shortcut is just > not good. When users click a button (unless it's a help button), they expect > something to happen. And still: We're putting something in the UI which is > used only a single time (afterwards the user knows that the function exists) > > Gregor Mi wrote: > 2. "Put it into the tooltip": Yes, sure this can and should be done. But > also see point 5. > > > 3. "Tooltips": oh, I indeed meant the Tooltips which you say are strongly > encouraged in the HIG. My mistake, thanks for clearing that! > > > 4a. "Clicking a button": the original idea was that clicking the button > actually triggers the function but for current technical reasons it was > postponed. So, adding the menu item now might motivate someone to go a step > further and implement the rest. One step at a time. > > 4b. "only used a single time": I myself am a user and due to the > outstanding stability of the desktop I keep forgetting these shortcuts. ;-) > Seriously, often I found myself in a situation where I knew there is a > shortcut but could not remember which (and previously I was lost completely > because I did not know that the killing window function exists at all). By > now I think I will never forget it, though. :) > > 5. Funnily, the first time I actually saw that the shortcut is documented > in the toolip was when I began to change the code to prepare this RR to make > the shortcut more visible. :) Sure, this only a single user report but I > don't think I am the only who does not read the whole tooltip. > > Ken Vermette wrote: > As I see it here, the current solution isn't good and I'd argue against > changing things just because they feel stale. Mainly you're trading one > less-than-ideal solution (using a tooltip) in exhange for a solution which is > more complicated and bloats the UI. Why do we need to go so far out of our > way to advertise XKill when we are capable of killing apps from the monitor? > Users of the monitor familliar with the system chould not have purely > informational menus burned into the main UI. > > Split buttons are also abused regularly, and it's not clear what the "V" > menu offers until activated. Why would someone who won't stop for a tooltip > know to press the "V" button? What if they assume it's for other actions like > pause, suspend, and resuming the selected process? Users *still* won't click > it if that's the case. This is essentially a much more convoluted tooltip > which works under the assumption users will investigate it because it's > disguised as functionality. > > I also dislike that it advertised as a feature and not a help option, and > users with a crashing/frozen application will only be more frusterated if > they think they have a solution - and are instead given a manual. Nowhere > else do we use this pattern. Information should NEVER be disquised as > functionality; it **severly** degrades user trust. Imagine if every > application did this - would you use a system which kept popping up with info > boxes instead of doing the work? Especially when you *need* the system to > take care of a problem and the system tells you "I can't do this, try doing > this" - I can think of no better way to shatter a users trust; one thing is > already broken, we should not make it feel *m
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
> On Jan. 23, 2016, 5:31 p.m., Gregor Mi wrote: > > > If someone has changed the shortcut, they should know what shortcut they > > > set it to, right? So having the tooltip just say "To kill a specific > > > window, press the "Kill Window" shortcut (Ctrl-Alt-Esc by default)" > > > should do the trick, right? > > > > In principle, I agree with you here. :) But... > > > > I have these premises in mind: > > > > 1. There might be a situation where the user can't remember what he has > > done. > > 2. I prefer precise over approximate information if it is reasonable easy > > to get (by fixing this https://git.reviewboard.kde.org/r/122981/ it is now > > easily possible and the patch is already written) > > 3. Somewhere I read that tooltips are to be avoided where possible => this > > patch factors some information out of a tooltip which in itself is > > desirable. > > 4. For the user, the discoverability of the feature after applying this > > patch is better than before (though not perfect, sure). > > Thomas Pfeiffer wrote: > 2. If we can get the current shortcut, why can't we get it for the > tooltip? > 3. Wait, are we talking about different tooltips? I do _not_ mean the one > you get when clicking the ? icon in the window decoration. Those should be > avoided because few people even notice that button. What I mean is the thing > that pops up when hovering over a control. Those are strongly encouraged in > the HIG, in fact they should be used on every control where the function > isn't clear from the label. > 4. Clicking a button just to get explained how to use a shortcut is just > not good. When users click a button (unless it's a help button), they expect > something to happen. And still: We're putting something in the UI which is > used only a single time (afterwards the user knows that the function exists) > > Gregor Mi wrote: > 2. "Put it into the tooltip": Yes, sure this can and should be done. But > also see point 5. > > > 3. "Tooltips": oh, I indeed meant the Tooltips which you say are strongly > encouraged in the HIG. My mistake, thanks for clearing that! > > > 4a. "Clicking a button": the original idea was that clicking the button > actually triggers the function but for current technical reasons it was > postponed. So, adding the menu item now might motivate someone to go a step > further and implement the rest. One step at a time. > > 4b. "only used a single time": I myself am a user and due to the > outstanding stability of the desktop I keep forgetting these shortcuts. ;-) > Seriously, often I found myself in a situation where I knew there is a > shortcut but could not remember which (and previously I was lost completely > because I did not know that the killing window function exists at all). By > now I think I will never forget it, though. :) > > 5. Funnily, the first time I actually saw that the shortcut is documented > in the toolip was when I began to change the code to prepare this RR to make > the shortcut more visible. :) Sure, this only a single user report but I > don't think I am the only who does not read the whole tooltip. > > Ken Vermette wrote: > As I see it here, the current solution isn't good and I'd argue against > changing things just because they feel stale. Mainly you're trading one > less-than-ideal solution (using a tooltip) in exhange for a solution which is > more complicated and bloats the UI. Why do we need to go so far out of our > way to advertise XKill when we are capable of killing apps from the monitor? > Users of the monitor familliar with the system chould not have purely > informational menus burned into the main UI. > > Split buttons are also abused regularly, and it's not clear what the "V" > menu offers until activated. Why would someone who won't stop for a tooltip > know to press the "V" button? What if they assume it's for other actions like > pause, suspend, and resuming the selected process? Users *still* won't click > it if that's the case. This is essentially a much more convoluted tooltip > which works under the assumption users will investigate it because it's > disguised as functionality. > > I also dislike that it advertised as a feature and not a help option, and > users with a crashing/frozen application will only be more frusterated if > they think they have a solution - and are instead given a manual. Nowhere > else do we use this pattern. Information should NEVER be disquised as > functionality; it **severly** degrades user trust. Imagine if every > application did this - would you use a system which kept popping up with info > boxes instead of doing the work? Especially when you *need* the system to > take care of a problem and the system tells you "I can't do this, try doing > this" - I can think of no better way to shatter a users trust; one thing is > already broken, we should not make it feel *m
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
> On Jan. 23, 2016, 5:31 p.m., Gregor Mi wrote: > > > If someone has changed the shortcut, they should know what shortcut they > > > set it to, right? So having the tooltip just say "To kill a specific > > > window, press the "Kill Window" shortcut (Ctrl-Alt-Esc by default)" > > > should do the trick, right? > > > > In principle, I agree with you here. :) But... > > > > I have these premises in mind: > > > > 1. There might be a situation where the user can't remember what he has > > done. > > 2. I prefer precise over approximate information if it is reasonable easy > > to get (by fixing this https://git.reviewboard.kde.org/r/122981/ it is now > > easily possible and the patch is already written) > > 3. Somewhere I read that tooltips are to be avoided where possible => this > > patch factors some information out of a tooltip which in itself is > > desirable. > > 4. For the user, the discoverability of the feature after applying this > > patch is better than before (though not perfect, sure). > > Thomas Pfeiffer wrote: > 2. If we can get the current shortcut, why can't we get it for the > tooltip? > 3. Wait, are we talking about different tooltips? I do _not_ mean the one > you get when clicking the ? icon in the window decoration. Those should be > avoided because few people even notice that button. What I mean is the thing > that pops up when hovering over a control. Those are strongly encouraged in > the HIG, in fact they should be used on every control where the function > isn't clear from the label. > 4. Clicking a button just to get explained how to use a shortcut is just > not good. When users click a button (unless it's a help button), they expect > something to happen. And still: We're putting something in the UI which is > used only a single time (afterwards the user knows that the function exists) > > Gregor Mi wrote: > 2. "Put it into the tooltip": Yes, sure this can and should be done. But > also see point 5. > > > 3. "Tooltips": oh, I indeed meant the Tooltips which you say are strongly > encouraged in the HIG. My mistake, thanks for clearing that! > > > 4a. "Clicking a button": the original idea was that clicking the button > actually triggers the function but for current technical reasons it was > postponed. So, adding the menu item now might motivate someone to go a step > further and implement the rest. One step at a time. > > 4b. "only used a single time": I myself am a user and due to the > outstanding stability of the desktop I keep forgetting these shortcuts. ;-) > Seriously, often I found myself in a situation where I knew there is a > shortcut but could not remember which (and previously I was lost completely > because I did not know that the killing window function exists at all). By > now I think I will never forget it, though. :) > > 5. Funnily, the first time I actually saw that the shortcut is documented > in the toolip was when I began to change the code to prepare this RR to make > the shortcut more visible. :) Sure, this only a single user report but I > don't think I am the only who does not read the whole tooltip. > > Ken Vermette wrote: > As I see it here, the current solution isn't good and I'd argue against > changing things just because they feel stale. Mainly you're trading one > less-than-ideal solution (using a tooltip) in exhange for a solution which is > more complicated and bloats the UI. Why do we need to go so far out of our > way to advertise XKill when we are capable of killing apps from the monitor? > Users of the monitor familliar with the system chould not have purely > informational menus burned into the main UI. > > Split buttons are also abused regularly, and it's not clear what the "V" > menu offers until activated. Why would someone who won't stop for a tooltip > know to press the "V" button? What if they assume it's for other actions like > pause, suspend, and resuming the selected process? Users *still* won't click > it if that's the case. This is essentially a much more convoluted tooltip > which works under the assumption users will investigate it because it's > disguised as functionality. > > I also dislike that it advertised as a feature and not a help option, and > users with a crashing/frozen application will only be more frusterated if > they think they have a solution - and are instead given a manual. Nowhere > else do we use this pattern. Information should NEVER be disquised as > functionality; it **severly** degrades user trust. Imagine if every > application did this - would you use a system which kept popping up with info > boxes instead of doing the work? Especially when you *need* the system to > take care of a problem and the system tells you "I can't do this, try doing > this" - I can think of no better way to shatter a users trust; one thing is > already broken, we should not make it feel *m
Re: Review Request 126851: Places data engine: Rename model role name "index" to "id"
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126851/#review91568 --- Ship it! on the fence about this. it's a rename of a semi public thing that may cause problems, but if it breaks the qml model uhm, ok - Marco Martin On Jan. 23, 2016, 2 p.m., Daniel Faust wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126851/ > --- > > (Updated Jan. 23, 2016, 2 p.m.) > > > Review request for kde-workspace and Plasma. > > > Repository: plasma-workspace > > > Description > --- > > I wrote a qml applet using the places data engine and noticed that I can't > access the index variable of ListView items because it gets overwritten by > the places model. > This patch renames the "index" role name to "id" in order to avoid this > naming conflict. > > > Diffs > - > > dataengines/places/org.kde.places.operations 98a951d > dataengines/places/placeservice.cpp e0c93c5 > dataengines/places/placesproxymodel.h 467fe83 > dataengines/places/placesproxymodel.cpp 5ea807b > > Diff: https://git.reviewboard.kde.org/r/126851/diff/ > > > Testing > --- > > I couldn't find any other model that is using the places data engine so I > hope renaming the model role is fine. > The provided model still works and I tested the "Setup Device" and "Teardown > Device" operations of the service. > The operations "Show" and "Hide" don't seem to work anyway and the others > were not tested. > > > Thanks, > > Daniel Faust > >
Re: Review Request 122249: libksysguard: add Kill Window to End Process button and show correct keyboard shortcut
> On Jan. 23, 2016, 6:31 p.m., Gregor Mi wrote: > > > If someone has changed the shortcut, they should know what shortcut they > > > set it to, right? So having the tooltip just say "To kill a specific > > > window, press the "Kill Window" shortcut (Ctrl-Alt-Esc by default)" > > > should do the trick, right? > > > > In principle, I agree with you here. :) But... > > > > I have these premises in mind: > > > > 1. There might be a situation where the user can't remember what he has > > done. > > 2. I prefer precise over approximate information if it is reasonable easy > > to get (by fixing this https://git.reviewboard.kde.org/r/122981/ it is now > > easily possible and the patch is already written) > > 3. Somewhere I read that tooltips are to be avoided where possible => this > > patch factors some information out of a tooltip which in itself is > > desirable. > > 4. For the user, the discoverability of the feature after applying this > > patch is better than before (though not perfect, sure). > > Thomas Pfeiffer wrote: > 2. If we can get the current shortcut, why can't we get it for the > tooltip? > 3. Wait, are we talking about different tooltips? I do _not_ mean the one > you get when clicking the ? icon in the window decoration. Those should be > avoided because few people even notice that button. What I mean is the thing > that pops up when hovering over a control. Those are strongly encouraged in > the HIG, in fact they should be used on every control where the function > isn't clear from the label. > 4. Clicking a button just to get explained how to use a shortcut is just > not good. When users click a button (unless it's a help button), they expect > something to happen. And still: We're putting something in the UI which is > used only a single time (afterwards the user knows that the function exists) > > Gregor Mi wrote: > 2. "Put it into the tooltip": Yes, sure this can and should be done. But > also see point 5. > > > 3. "Tooltips": oh, I indeed meant the Tooltips which you say are strongly > encouraged in the HIG. My mistake, thanks for clearing that! > > > 4a. "Clicking a button": the original idea was that clicking the button > actually triggers the function but for current technical reasons it was > postponed. So, adding the menu item now might motivate someone to go a step > further and implement the rest. One step at a time. > > 4b. "only used a single time": I myself am a user and due to the > outstanding stability of the desktop I keep forgetting these shortcuts. ;-) > Seriously, often I found myself in a situation where I knew there is a > shortcut but could not remember which (and previously I was lost completely > because I did not know that the killing window function exists at all). By > now I think I will never forget it, though. :) > > 5. Funnily, the first time I actually saw that the shortcut is documented > in the toolip was when I began to change the code to prepare this RR to make > the shortcut more visible. :) Sure, this only a single user report but I > don't think I am the only who does not read the whole tooltip. > > Ken Vermette wrote: > As I see it here, the current solution isn't good and I'd argue against > changing things just because they feel stale. Mainly you're trading one > less-than-ideal solution (using a tooltip) in exhange for a solution which is > more complicated and bloats the UI. Why do we need to go so far out of our > way to advertise XKill when we are capable of killing apps from the monitor? > Users of the monitor familliar with the system chould not have purely > informational menus burned into the main UI. > > Split buttons are also abused regularly, and it's not clear what the "V" > menu offers until activated. Why would someone who won't stop for a tooltip > know to press the "V" button? What if they assume it's for other actions like > pause, suspend, and resuming the selected process? Users *still* won't click > it if that's the case. This is essentially a much more convoluted tooltip > which works under the assumption users will investigate it because it's > disguised as functionality. > > I also dislike that it advertised as a feature and not a help option, and > users with a crashing/frozen application will only be more frusterated if > they think they have a solution - and are instead given a manual. Nowhere > else do we use this pattern. Information should NEVER be disquised as > functionality; it **severly** degrades user trust. Imagine if every > application did this - would you use a system which kept popping up with info > boxes instead of doing the work? Especially when you *need* the system to > take care of a problem and the system tells you "I can't do this, try doing > this" - I can think of no better way to shatter a users trust; one thing is > already broken, we should not make it feel *m
