OpenWith - configuration dialog

Review Request #106388 - Created Sept. 9, 2012 and submitted

Information
Przemek Czekaj
kdevplatform
master
287764
Reviewers
kdevelop
Added a configuration dialog to the context menu.

@edit
give me few days, I will improve patch.
Compiled, and local install. Tested by hand.

Issues

  • 17
  • 2
  • 0
  • 19
Description From Last Updated
what's that comment for? :) Sven Brauch Sven Brauch
I'd propose to sort those in Qt/KDE and use one consistent include style Sven Brauch Sven Brauch
The formatting you use here is inconsistent with what you wrote above (spaces around function calls,...); it's not very consistent ... Sven Brauch Sven Brauch
Isn't this usually done automatically by the Qt ownership system? Sven Brauch Sven Brauch
either remove it or remove the typo at least :) Sven Brauch Sven Brauch
remove whitespace change Sven Brauch Sven Brauch
remove whitespace change Sven Brauch Sven Brauch
why use dynamic_cast? Sven Brauch Sven Brauch
remove whitespace change Sven Brauch Sven Brauch
This should be using KDialog::Ok, since the dialog will be closed when clicking it. Apply is for situations where the ... Andreas Pakulat Andreas Pakulat
This should use okClicked and then not use close() in the save() slot. Andreas Pakulat Andreas Pakulat
Why is the selection stored in a (not quite well named) variable? You could just query it in save(). Also ... Andreas Pakulat Andreas Pakulat
This could be directly connected to the setDisabled slot of the services widget. No need for a separate slot here. Andreas Pakulat Andreas Pakulat
Where does this locale setting come from and what consequences does it have? Andreas Pakulat Andreas Pakulat
This layout is unecessary, you're merely nesting a vertical layout in another which does not make any sense. Andreas Pakulat Andreas Pakulat
What does this option do? If its about opening the file embedded then why have a checkbox instead of listing ... Andreas Pakulat Andreas Pakulat
renamed Przemek Czekaj Przemek Czekaj
Sven Brauch
Andreas Pakulat
Przemek Czekaj
Milian Wolff
Przemek Czekaj
Przemek Czekaj
Przemek Czekaj
Przemek Czekaj
Przemek Czekaj
Commit Hook
Przemek Czekaj
Review request changed

Status: Closed (submitted)

Milian Wolff
note: this patch was actually leaking the config dialog, I've fixed that now and cleaned up the whole plugin as well

cheers, many thanks.
  1. hm actually looking at the stuff a bit more, I think I'll remove the config dialog again and simplify the ui a bit such that it should be more straight forward and not require a config dialog...
    
    sorry to have wasted your time and thanks for your contribution anyways. try to review my changes afterwards, maybe you learn something or have suggestions :)
    
    cheers
Loading...