GNOME Bugzilla – Bug 634090
Change some attributes to local variables
Last modified: 2011-09-19 20:17:58 UTC
I have noticed that GParted classes contain private attributes which are used only by a single member function. Examples: - Gtk::Image* image_temp http://git.gnome.org/browse/gparted/tree/include/Dialog_Base_Partition.h?id=d040e4e5847905f58207e093917b6c3d36e641d7#n103 - Glib::ustring str_temp http://git.gnome.org/browse/gparted/tree/include/Dialog_Progress.h?id=d040e4e5847905f58207e093917b6c3d36e641d7#n99 - Gtk::HBox* hbox2 http://git.gnome.org/browse/gparted/tree/include/DialogFeatures.h?id=d040e4e5847905f58207e093917b6c3d36e641d7#n45 I suggest to move such instances to the corresponding function implementations to stress their usage scope. This kind of refactoring will also affect the issue "Improve implementation of constructors" (bug #633635) ... ;-)
These are some excellent suggestions to improve the code base. If you would like to develop a patch for this improvement, then I will plan to include the patch in an upcoming GParted release.
Created attachment 173968 [details] [review] update suggestion Do you see any unexpected effects because of the suggested changes? Would you like to integrate the appended adjustment into your source code repository?
Created attachment 189417 [details] File System Support Dialog prior to Patch Markus, Thank you for your work on this patch and your interest in GParted. If you should choose to ignore this follow-up I would understand. After all it has been several months since you first posted this patch and I have not responded until now. Today I applied the patch against the current code in the git repository. The patch applies cleanly with the exception of a recent change in wording from "amount" to "number" in src/Dialog_Progress.cc. See Bug #650237 - Grammar error: amount -> number After applying the patch and compiling the code using kubuntu 11.04, I proceeded to start testing. My expectation was that the GParted application would continue to function exactly the same as prior to applying this patch. The first dialog in the list was DialogFeatures.cc, so I used menu "View -> File System Support" to check to ensure that the dialog worked as before. I was surprised to discover that the expandable "Legend" in the lower left hand corner was missing (see attachment with screen shot prior to patch). At this point I stopped testing because I would be unable to use the patch as it currently exists. If this patch addressed an application crash, or fixed some other feature I would have had more incentive to investigate further. If I recall correctly this patch is to make the code cleaner, which is a good thing to do, but not (IMHO) if it adversely impacts the application. My thoughts on how to proceed are in conflict because I realize that you did significant work to create this patch. My concern is that this patch is not well tested and the risk of using it appears to be higher than the risk of not using the patch. I had hoped that I would be able to quickly test it and apply the patch to the git repository. Unfortunately this patch looks like it requires much more testing, and involves more risk than I am willing to take for a patch to cleanup code. If this bug report is something that you would like to continue working on, then perhaps it would be better to break it into much smaller portions. For example, keep the changes as small as possible and limited to as few files as possible. This would make it easier to review the proposed changes and to perform testing. If you should choose not to work on this bug report, then I completely understand your decision. In such a case I hope that other projects can benefit from your enthusiasm and efforts to improve free open source code. Did you want to continue working on this bug report?
(In reply to comment #3) > Did you want to continue working on this bug report? Yes. - I would like to continue this issue if you are also still interested in a clarification for the usage of the mentioned class attributes.
Yes, I would like to continue to resolve this issue and improve the code. To start, what do you think of developing a patch for the "View -> File System Support" portion of the application? I think if we do the work in small pieces then it will be easier for me to review, test, and then commit the work.
(In reply to comment #3) > I was surprised to discover that the expandable "Legend" in the lower left hand > corner was missing (see attachment with screen shot prior to patch). I'm sorry that I did not miss the display of this control element in the feature dialogue during my testing.
Created attachment 190691 [details] [review] update suggestion How are the chances for integration of this adjustment into your source code repository?
Thank you Markus for the reworked patch. This patch has been applied to the GNOME git repository for inclusion in the next release of GParted. The relevant git commits can be viewed at the following links: Bug #634090: Change some attributes to local variables http://git.gnome.org/browse/gparted/commit/?id=91b971691d912859cbeb3b916c27bb90c1fcceec Update AUTHORS file and minor variable rename http://git.gnome.org/browse/gparted/commit/?id=813010b148e40f3b2e313bb079ffc960e0d2ce2c
This enhancement has been included in the GParted 0.9.1 release on September 19, 2011. Closing this bug report.