[PATCH] BUG: 178658 Adding remote encoding support to Dolphin
peter.penz at gmx.at
Wed Mar 25 07:59:18 GMT 2009
Thanks for the patch, this was on my TODO list for several months already but I did not code a single line yet...
> On Tue, Mar 24, 2009 at 11:52 PM, Thiago Macieira <thiago at kde.org> wrote:
> >> Patch looks good, but before you commit, can you fix your indentation
> >> problems? Please use the same indentation as the rest of Dolphin
> >> appears to be using 4-space indentation but your new file is doing
> >> 2-then-4
> >> and some braces are in weird positions).
> Here is a clean one. Properly indentated. So should I OK to commit
I think the patch looks good and should be committed. But I'd like to wait for David Faure's input too, as I also think we must prevent code duplication and Konqueror should just use the Dolphin KPart for this.
Regarding the patch: I did not check the DolphinRemoveEncoding class in detail (as you said it's based on Thiago's kremoteencodingplugin and Thiago already reviewed it). Some minor details I found (nitpicking stuff):
- dolphinui.rc: Could you please rename the new item to "change_remote_encoding" instead of "changeremoteencoding" to stay consistent with the other names?
- dolphinviewactionhandler.cpp: You include "dolphinnewmenuobserver.h", but have not used it (maybe a relict from the recent "Create New..." patch?).
- dolphinviewactionhandler.cpp: This very nitpicking, but I always try to keep the same order of the methods in the cpp file like in the h-file -> could you please move the new methods currentView() and actionCollection() below the same methods like in the h-file? Thanks ;-)
> Rahman Duran
> Software Engineer
> How many apples fell on Newton's head before he took the hint!
More information about the kde-core-devel