GNOME Bugzilla – Bug 691681
Improve Partition Info dialog to be Partition properties dialog
Last modified: 2015-02-01 18:39:22 UTC
Improve Partition Information Dialog to be abled to edit these partition properties: -> filesystem type -> Partition label (see #690953) -> file system label -> file system UUID The ability to edit these properties can then be removed from the main window. To forestall confusion, there should be some separation between the partition properties and the ones of the filesystem. So elements should be ordered like this: headline: "file system" file system label used/unused status file system uuid (blank line) headline: "partition" path size Partition label file system flags (blank line) sector information (3 lines) The bug was created as a result of discussion in #690953. I'll do the coding work, Curtis updates the manual, as he stated in Comment 4 of #690953. Greetings, Sinlu Bes
Thanks Sinlu for creating this new report to track this enhancement. I have updated the plans for the next release of GParted accordingly. See: Development Plans for the Next Release of GParted (0.15.0) http://gparted-forum.surf4.info/viewtopic.php?id=16716 Please note that it is not likely that all of these bug reports will be completed for the next release. This post is simply a list of candidates that are in various stages of work. When the code for this enhancement is substantially complete I will start on updating the documentation. Both Mike and I watch over the bug reports, so feel free to ask if you have any questions.
Created attachment 233689 [details] [review] version 1 - mostly gui features, no operation creation implemented yet Hello, Could you please review my unfinished patch? GUI seems to work now. The function in Win_GParted to apply the changes is not implemented yet, so clicking on "save" won't have another effect than clicking on "close". Having Entries and an OptionMenu around in Partition properties, the horizontal spaces are irregular, and therefore the dialog is not very pretty. Only the height of the Checkbox is small enough to fit in. So i think it would be best when the entry boxes for filesystem and partition label and the filesystem type OptionMenu are hidden first, with an italic label each instead. This makes it nicer to look at. If the user wants to edit the label or filesystem type, he has to double-click onto the label, which converts it into an entry/OptionMenu. Is this concept user friendly? Is some hint needed? Is it too complicated? What about keyboard-only access? I thought of adding a Button labeled "edit all", which does the same thing as double-clicking on every editable property. The problem is that such a button would fit best next to "save" and "close", but I don't see a way of adding a button at this position that doesn't close the dialog immediately. What's your idea? Greetings, Sinlu Bes
Hi Sinlu, Thank you for this initial patch. I think it is a good start on this enhancement. I particularly like the check box for "Change UUID". I think we can use something similar for "Reformat partition" to ensure that users do not accidentally format over the existing partition. QUESTION: > Is this concept user friendly? Is some hint needed? Is it too complicated? > What about keyboard-only access? ANSWER: With GParted we try to follow the GNOME Human Interface Guidelines. We use these guidelines to help us with user interface decisions. See, http://developer.gnome.org/hig-book/2.32/ In my following comments I will try to use the GNOME HIG to guide our changes to the Properties dialog window. 1) It should be obvious to the user which properties can be changed. I think it is non-intuitive to double-click on the value of an italic field to be able to edit the value. Instead we should display editable properties directly in editable controls. See, http://developer.gnome.org/hig-book/2.32/windows-utility.html.en#property-windows To determine if an editable control value has changed, we can compare the original setting of a label to the value when the "Apply" button is clicked. 2) We should use bold text on the column and section headings only. In the properties dialog, we should use bold for the "File System" and "Partition" section headings (already done). The other values under these sections should be in normal unbolded font, such as "Used:" and "Status:". See, http://developer.gnome.org/hig-book/2.32/design-window.html.en#layout-dialogs Note that I realize the field labels should not have been bolded in the original Information dialog. 3) We should use proper capitalization. Headings use heading capitalization (e.g, "File System"), whereas field labels use sentence capitalization (e.g., "File system".) See, http://developer.gnome.org/hig-book/2.32/design-text-labels.html.en 3) We should not use frames or lines to separate sections. We should remove the lines under "File System" and "Partition". The preferred method is to use spacing and bold headers to indicate the sections. See, http://developer.gnome.org/hig-book/2.32/design-window.html.en#window-layout-spacing 4) We should be consistent with button names and placement. For consistency We should probably use the names "Cancel" and "Apply" in that order at the bottom of the properties dialog. For consistency with GParted I am referring to the "Device --> Create Partition Table" window as a template. See, http://developer.gnome.org/hig-book/2.32/windows-utility.html.en#windows-explicit-apply Please note that in GParted we often use the button name "Apply" when the GNOME HIG would indicate to use the name "OK". I believe this was originally done to stress that changes will occur if the user clicks the button. My personal opinion is that this was a reasonable decision by the original developer of GParted. 5) We should group similar items together. I think we should move the drop-down list for "File system" up to the the file system section. Also I think we should move "Status" to the partition section, and "Size" to be with "Used" and "Unused" in the file system section. For example, here is a mock-up of the proposed order for the fields: *File System* Label: editable-label UUID: xxx X checkbox for "Change UUID" File system: drop-down list for FS X checkbox for "Reformat partition" Used: xxx Unused: xxx Size: xxx *Partition* Label: editable-label Path: xxx Status: xxx Flags: xxx First sector: xxx Last sector: xxx Total sectors: xxx Cancel button Apply button Note that this mock-up places the more likely to be used editable controls higher in the field label list. Also the asterisks have been used to indicate these headings should be in bold. See, http://developer.gnome.org/hig-book/2.32/windows-dialog.html.en#dialog-layout Please feel free to question my responses as I make mistakes too. :-)
Hi Curtis, thank you for your remarks. Having a checkbox for reformatting the filesystem does not just ensure that the user won't accidentally remove all data by just scrolling the mouse when being over the drop-down list, it also enables him to reformat to the same filesystem. I've already asked myself of how to do that. This is a good idea. 1) Creating the double-click -> edit wasn't motivated by the problem to know whether the user wants to edit, but simply out of consistency in horizontal spacing. Perhaps a hint somewhere would clear the user's confusion a little bit, but you're right, this is not as user-friendly as the direct display of entries. 2) bold text only in headlines In the guideline you cited in 1), they use bold labels for the entries. I think this is because bold labels help to distinguish between the name and the value of a property. In the other guideline you cited in 2), the property names do not need to be bold as there are no read-only properties displayed by labels. Partition properties dialog has mixed content of read-only and read-write values. So we need a way to both distinguish between property name and value, and property name and category label. In the first guideline they used tabs to separate the properties of a category. Tabs are not a solution for partition properties dialog because it contains too few elements per category. Perhaps a solution would be to make to make the headline labels bold and italic the same time. 3a) Heading capitalisation is a detail i didn't thought of. Thank you for calling attention. 3b) I see, identation is a better solution than spacers. 4) I agree to renaming the buttons, but i think their order should remain. Users accustomed to old GParted will perhaps run into accidental edits when they click onto the "right bottom corner" of the window to close it, not being aware that there is now the apply button. I realize this causes inconsistencies to "new partition" dialog which cannot be solved by changing the button order of that dialog, because there the users are accustomed, too. 5) On setting the order for my patch i thought that: a) size has perhaps a better place next to used and unused when opening a filesystem, but what about opening an unallocated part of the disk? There the size line would be the only element in the then needed filesystem section, which an unallocated part hasn't. I could live with the small inconsistency of having the label in the filesystem section when opening a partition and having it in the partition section otherwise. b) you mount the filesystem and not the partition (regarding Status label) The the other changes to the order you propose seem ok for me.
Hi Sinlu, (In reply to comment #4) > 2) bold text only in headlines Good points. I can see that the combination of read-only and editable properties along with section headings is not directly addressed in the HIG documentation. > Perhaps a solution would be to make to make the headline labels bold and > italic the same time. This could work, but I would prefer to use something that more closely follows the GNOME HIG. The HIG says to use spacing and indentation to separate sections. I do recall reading the following somewhere: If there are section headers present, labels should be indented. What about using bold for both the headers and field labels, and further indenting the field labels to the right? For an example of the indentation see, http://developer.gnome.org/hig-book/2.32/design-window.html.en#layout-callouts-figure > 4) Consistent button names and placement > > I agree to renaming the buttons, but i think their order should remain. > Users accustomed to old GParted will perhaps run into accidental edits when > they click onto the "right bottom corner" of the window to close it, not being > aware that there is now the apply button. > > I realize this causes inconsistencies to "new partition" dialog which cannot > be solved by changing the button order of that dialog, because there the > users are accustomed, too. For users that are accustomed to the old Information dialog, they probably would not change the values. In this case, pressing the "Apply" button will have no effect since the users did not change any values. The only time that the placement of the buttons has any effect is if the user changes a value. In this situation I would hope they would read the button name before just blindly clicking on it. At some point the users will need to learn how the new properties dialog works. When the user clicks "Apply" the actions are still just queued, and have not been written to disk yet. As such the queued operations can be undone. For consistency across dialogs I still prefer to use the order "Cancel" followed by "Apply". > 5) Group similar items together > > On setting the order for my patch i thought that: > > a) size has perhaps a better place next to used and unused when opening a > filesystem, but what about opening an unallocated part of the disk? > There the size line would be the only element in the then needed filesystem > section, which an unallocated part hasn't. > I could live with the small inconsistency of having the label in the > filesystem section when opening a partition and having it in the partition > section otherwise. In the old interface, opening an unallocated portion of the disk would display the following: File system: unallocated Size: xxx First sector: xxx Last sector: xxx Total sectors: xxx With the new interface it will be similar, but with the "File System" and "Partition" headings. I think that this should be okay. > b) you mount the filesystem and not the partition (regarding Status label) Good point. I agree. Hence the status field should probably be in the "File System" section.
Created attachment 234492 [details] [review] version 2 - everything implemented Hi Curtis, I updated the patch and added write support. Greetings Sinlu
Hi Sinlu, I just took a quick peek at the patch in comment #6, by applying and running the code. This enhancement is looking better each time I look. :-) Following are some more suggestions: 1) Remove the italics formatting from the "File System" and "Partition" titles. I think that the indentation is sufficient to indicate that these are titles. Also I think that removing the italics will better match the GNOME HIG. 2) Remove the words "File system" from in front of the "File system Label" and "File system UUID". Since these fields are under the "File System" section, it should be assumed that these fields are file system related. 3) Remove the word "Partition" from in front of the "Partition Label". Since this field is under the "Partition" section, it should be assumed that this label refers to a partition label. 4) Move the "File system" field and corresponding "Reformat file system" checkbox up to the "File System" section. Since these are file system related, I think these belong under the "File System" section. 5) Keep the "Label" under the "File System" section editable when the user activates the "Reformat file system" checkbox. This will permit users to reformat the file system with a label, as opposed to setting a blank label. By the way, I like the way you only activate the "File system" drop-down list when the checkbox is enabled. :-) Please feel free to disagree with my suggestions if you think these are incorrect. Also I haven't looked closely at the code yet, so later I might have some suggestions in this area.
In comment #7 I need to expand on point (5). I noticed this behaviour when selecting "Reiser4" as the file system when the "Reformat file system" checkbox was enabled. When reformatting a file system, this should be performed in one step. I'm guessing that it might currently be two steps: reformat, then label. And since labelling Reiser4 is not supported that is likely why you blanked out the file system label. A label can be set when Reiser4 is initially formatted.
Hi Curtis, thank you for your suggestions. 1-4) These are changes I can live with. 5) You are right concerning your guesses in comment #8. The dialog splits all changes into multiple operations. I see, labelling a partition while formatting is a feature not supported by gparted yet. I have two ideas to implement this feature: A) change the code of all formatting Operations to set also the label. B) copy and modify the existing Operation to create an Operation like in A), for all filesystems that can only have label settable on creation. For all other filesystems, the old method (two separate Operations) can be used as fallback. B) allows a cleaner separation between filesystem creation and filesystem creation with labelling as some filesystems like lvmp2 do not have any labels readable by gparted. Are there other filesystems than reiser4 which support setting of label while partitioning?
Hi Sinlu, (In reply to comment #9) > 5) > You are right concerning your guesses in comment #8. The dialog splits all > changes into multiple operations. > I see, labelling a partition while formatting is a feature not supported by > gparted yet. You raise a valid point. The current GUI for format does not support setting the label when reformatting the file system. However, the underlying FileSystem class method create() _does_ support setting the label while formatting. With your new Properties dialog window, there is now a place for the user to specify a label, so we could make the necessary code updates so that the reformat operation also sets the label while formatting. > I have two ideas to implement this feature: > A) change the code of all formatting Operations to set also the label. > B) copy and modify the existing Operation to create an Operation like in A), > for all filesystems that can only have label settable on creation. > For all other filesystems, the old method (two separate Operations) can be used > as fallback. > > B) allows a cleaner separation between filesystem creation and filesystem > creation with labelling as some filesystems like lvmp2 do not have any labels > readable by gparted. Regarding (A), both the "Partition --> New" operation, and the "Partition --> Format" operation use the same underlying command to actually format the file system. The format command is contained in the FileSystem::create() method. As such we should not need to change the formatting operations for each file system. In other words, if the file system label is set in the Partition class, then the file system will be formatted with a label. Based on this, I do not think that we need to consider (B) in order to have a cleaner separation between some file system label setting limitations. My thoughts are that it would be better to only perform the format operation when the checkbox to reformat is enabled. Otherwise it makes sense to queue separate operations, such as new UUID and set FS label when the file system is not being reformatted. Its a bit trickier with lvm2 pv because lvm2 pv is not really a file system. As such it does not set fs.read_label or fs.write_label. > Are there other filesystems than reiser4 which support setting of label while > partitioning? Yes. HFS, HFS+, and Reiser4 are currently supported file systems that can have the label set while formatting, but not changed after the file system exists. Other exceptions are LVM2 PV which is not really a file system, and exfat and ufs which are only partially supported and do not have fs.write_label ability. The logic in the Properties window might be: If checkbox to reformat is enabled, then permit setting of FS label for all file systems that also have fs.read_label ability. Else if checkbox to format is disabled, then permit setting of FS label only for those FS that have fs.write_label ability. I think this logic would permit us to simply reformatting, setting label, and new UUID down to one "format" operation. Does that logic sound reasonable to you?
Created attachment 235839 [details] [review] version 3 Hello Curtis, the patch has support for setting label and reformatting in one step now.
Hi Sinlu, Thank you for your patience. Today I started to look into your patch in comment #11. At the moment it does not cleanly apply, likely due to the major changes involved when the code base was refactored for bug #685740. Would you be able to rebase your patch against the current HEAD of the master branch? Also, when applying the patch I noticed that there was some trailing whitespace that should be cleaned up. Following is the log when I try to apply the patch: $ git am bug691681-partition-properties.patch Applying: Improve Partition Information Dialog to be Partition Properties Dialog (Bug #691681)(v3) /home/gedakc/workspace/gparted.dev2/.git/rebase-apply/patch:234: trailing whitespace. /home/gedakc/workspace/gparted.dev2/.git/rebase-apply/patch:1304: trailing whitespace. /home/gedakc/workspace/gparted.dev2/.git/rebase-apply/patch:1326: trailing whitespace. /home/gedakc/workspace/gparted.dev2/.git/rebase-apply/patch:1775: space before tab in indent. newestpartition, error: patch failed: src/Win_GParted.cc:1967 error: src/Win_GParted.cc: patch does not apply Patch failed at 0001 Improve Partition Information Dialog to be Partition Properties Dialog (Bug #691681)(v3) When you have resolved this problem run "git am --resolved". If you would prefer to skip this patch, instead run "git am --skip". To restore the original branch and stop patching run "git am --abort". $
Created attachment 239654 [details] [review] rebased Now the patch is rebased -- to the state of the git repo after applying patch for Bug 696471 . Greetings, Sinlu
Hi Sinlu, Until we know for sure whether it is a good idea to apply the remove all trailing white space patch (Bug #696471), would you be able to rebase your patch using the current git head? Thanks, Curtis
Created attachment 239716 [details] [review] rebased to be child of git 12d2cad682f1da06e2c59459ba73d2715cb1e615 The patch is rebased to 12d2cad682f1da06e2c59459ba73d2715cb1e615 now.
Created attachment 239718 [details] [review] resolved small issue This update resolves a small issue when having selected lvm2 pv in Property window, and then selecting reiser4.
Hi Sinlu, Thank you for your work on this significant enhancement. This is not a small undertaking and I appreciate your work in this area. Following are the results from my testing, and some suggestions and questions for further improvement. If you have any questions or comments then please don't hesitate to ask. This is a big patch and I might easily miss something important. Test Matrix =========== Change Set Reformat Change Actual Volume New File Partition Operation(s) Label UUID System Label Queued ------ ---- -------- --------- -------------------------- - - - - <none> Yes - - - Set FS Label - Yes - - Set UUID Yes Yes - - Set UUID + Set FS Label - - Yes - Format /dev Yes - Yes - Format /dev labeled as - Yes Yes - Format /dev Yes Yes Yes - Format /dev labeled as - - - Yes Set P Label Yes - - Yes Set P Label + Set FS Label - Yes - Yes Set P Label + Set UUID Yes Yes - Yes Set P Label + Set UUID + Set FS Label - - Yes Yes Set P Label + Format /dev Yes - Yes Yes Set P Label + Format /dev labeled as - Yes Yes Yes Set P Label + Format /dev Yes Yes Yes Yes Set P Label + Format /dev labeled as All of the above tests had the results I expected. Great work! Code Review =========== Since a large patch takes a long time to review, would you be able to place future enhancements in logically separate commits? For example, With this patch, it would have helped to have the rename of Dialog_Partition_Info.[h|cc] to Dialog_Partition_Properties.[h|cc] in a separate commit so that the enhancements you made would be readily visible. Questions and Suggestions for Improvement ----------------------------------------- 0) Rebase for current master From now on I will focus my attention on this patch to minimize the need to rebase. When the patch is ready I will commit it and then proceed to update the relevant sections in the GParted Manual. 1) Different include paths in Dialog_Partition_Properties.cc? I don't think that the "include/GParted_Core.h" path will work. Is this needed? If so it should probably have "../" prefixed to the path. + +#include "../include/Dialog_Partition_Properties.h" +#include "../include/LVM2_PV_Info.h" +#include "include/GParted_Core.h" + 2) Use same title "Properties of %1" for regardless of read-only status. In both cases the data represents properties of the partition, only in one case these properties are not editable. 3) Enhance "Warning" messages to be a scrollable area. With long messages, this can result in a window that is larger than the vertical display resolution. Note that I thought this was raised in a prior bug report, but I have not been able to find it. 4) Change "filesystem" to "file system" in all strings (not necessarily code) to be consistent with previous code. 5) Change "format" string text to remove comma. For example Change: _("Format %1 as %2, labeled as \"%3\"") To: _("Format %1 as %2 with label \"%3\"") 6) In the Win_GParted::Merge_Operations() when merging two file system label operations, why was a line added to set the partition_new.filesystem added? My thoughts were that the operations[] copy / equal setting should set all the relevant partition details. 7) Eventually we will also need to remove the newly redundant code. Specifically I am talking about the following menu operations: Partition --> Format Partition --> Label Partition --> New UUID However, let's leave this until we have the other parts of the patch set sorted out. The above are items that I noticed in the first pass of the code. There may be others that I might see on subsequent reviews. Again, please feel free to question any of my suggestions.
Hi Curtis, Thank you for reviewing my patch. Code Review Do you want every change in a new commit, or just the major ones, and the minors in an update to the old commit? When I have updated my patch you can run a simple and bare diff on it to see the changes. 1) It actually compiles without the include, too. I'll remove the line. 2) Yes, its true that the content remains to be properties the properties of a partition, even if its readable. The menu entry Partition --> Properties remains to be labelled "Properties", even if the partition is read-only. 3) Did you mean Bug #690542? I could write a patch, but I would prefer to do this after the commit for this bug is commited (to avoid rebasing), and I'd post it in that Bug, if you don't mind. In fact, the issue is not very related to the change from Information dialog to Properties dialog. However, this issue is important, as it is likely to use gparted on some linux-rescue-iso where the graphic drivers aren't the main priority, so you end up to use gparted with a low screen resolution, even if you have more modern hardware. I could put the Warnings ( the code names them "messages" ) into some scrollable TreeView-Object. There is already a fixme in the code demanding something similar. 4), 5) The changes sound reasonable to me. 6) which setting do you mean? The added line was to meet a small issue I explored: comment out the line, and then try the following: 1. relabel some partition 2. reformat the partition with another filesystem 3. relabel it again. you will see that the original operation from step 1 will be changed, but the filesystem label in the reformat operation will remain. 4. execute The partition will still have the name given in step 1. I see now, the change doesn't fully solve the issue, because it doesn't work when the reformat happens to the same filesystem. This problem wasn't an issue before the change, because back then you couldn't relabel a partition when there is a format operation pending. There are two ways about how to fix the issue: a) merge the relabel operation with the reformat operation b) add a separate relabel operation similar to the behaviour of the patch now. whats your opinion? As the changes were forbidden in the old version, this new feature could be a field of your tests. 7) The old Dialog_Partition_Info has to be removed, too. It doesn't get built already.
Hi Sinlu, Thank you for your prompt reply. My responses follow inline. (In reply to comment #18) > Code Review > Do you want every change in a new commit, or just the major ones, and the > minors in an update to the old commit? Your suggestion sounds good to me (minors in an update to an old commit). I will leave this up to you. If each logical change is in a commit it makes it easier to review the patchset. > When I have updated my patch you can run a simple and bare diff on it to see > the changes. This is a bit of extra work, but I will do this so that I can more easily see the changes. Thank you for the reminder of this tip. > 3) > Did you mean Bug #690542? Yes, that is exactly the bug report I was trying to find. :-) > I could write a patch, but I would prefer to do this after the commit for this > bug is commited (to avoid rebasing), and I'd post it in that Bug, if you don't > mind. > In fact, the issue is not very related to the change from Information dialog > to Properties dialog. Another good point. We should try to keep this patch set specific to what it is trying to resolve. Specifically to change the Partition Information window into a Partition Properties window that enables users to label, set new UUID, and reformat. Ideally the portion for setting a Partition Label (as opposed to the file system label) would be in Bug #690953 - Partition name support. However, since you have already done this work in this patch set there is no sense in undoing the work. > 6) > > which setting do you mean? I had meant the following code change: - // Two label change operations on the same partition + // Two filesystem label change operations on the same partition else if ( operations[ first ]->type == OPERATION_LABEL_PARTITION && operations[ second ]->type == OPERATION_LABEL_PARTITION && - operations[ first ]->partition_new == operations[ second ]->partition_original + operations[ first ]->partition_new == operations[ second ]->partition_original && + operations[ first ]->partition_new .filesystem == operations[ second ]->partition_new .filesystem Based on your following comments it seems you figured out my intentions. > The added line was to meet a small issue I explored: > comment out the line, and then try the following: > 1. relabel some partition > 2. reformat the partition with another filesystem > 3. relabel it again. you will see that the original operation from step 1 will > be changed, but the filesystem label in the reformat operation will remain. > 4. execute > The partition will still have the name given in step 1. > I see now, the change doesn't fully solve the issue, because it doesn't work > when the reformat happens to the same filesystem. > This problem wasn't an issue before the change, because back then you couldn't > relabel a partition when there is a format operation pending. > There are two ways about how to fix the issue: > a) > merge the relabel operation with the reformat operation > b) > add a separate relabel operation similar to the behaviour of the patch now. > whats your opinion? I like option (a) as this would improve upon the old functionality. > As the changes were forbidden in the old version, this new feature could be a > field of your tests. Another good point. I will need to add this to the tests. > 7) > The old Dialog_Partition_Info has to be removed, too. It doesn't get built > already. Good catch. We will need to remember this too when adding the cleanup of old code to this patch set.
Created attachment 240146 [details] [review] removed unneeded include, improved user-interface strings, fixed merge Hi Curtis, thank you for your fast reply :) I've included the changes for the points 1), 2), 4), 5), 6) into the patch. I've also fixed two whitespace-only-lines :) 6) option a) has less constellations to think of, too. I've removed Dialog_Partition_Info already now, because I think point 7) can get its dedicated patch, and the this code should be removed as the Partition Properties dialog is a modified Partition Information dialog, contrary to the other code to be removed. I've tried to make the changes into multiple patches, but it didn't work. However, if it helps, I've found a better way to compare two patch versions than just a simple diff: git diff 44114192b8c66a96050020b1ba2e3611651806c9 a5ba4504b9830727aae0f4454fef732bde7d97e3
of course it should be: git diff a5ba4504b9830727aae0f4454fef732bde7d97e3 7b11086569228c99c95c43e7846e43e1eddb3ecb
Hi Sinlu, Thank you for the updated patch. (In reply to comment #20) > I've tried to make the changes into multiple patches, but it didn't work. Assuming that you have made separate commits in a new branch while developing your changes, you can create a patch set using the following command: git format-patch origin/master -n --stdout > ~/mypatch.mbox If you have a chance to do this then it would be much appreciated. With a single large patch I will have to start reviewing from the start again which takes much more time.
Hi Sinlu, The patch in comment #20 does not apply cleanly for me. Following is the output: $ git am bug691681-partition-properties.patchApplying: Improve Partition Information Dialog to be Partition Properties Dialog (Bug #691681) error: patch failed: src/Win_GParted.cc:436 error: src/Win_GParted.cc: patch does not apply Patch failed at 0001 Improve Partition Information Dialog to be Partition Properties Dialog (Bug #691681) When you have resolved this problem run "git am --resolved". If you would prefer to skip this patch, instead run "git am --skip". To restore the original branch and stop patching run "git am --abort" Perhaps this patch is based on some of your prior work to remove all of the trailing whitespace from all GParted files?
Created attachment 240374 [details] [review] rebased to newest head and removed unneeded include, improved user-interface strings, fixed merge Hi Curtis, The patch from comment #20 was based on the git commit: 12d2cad682f1da06e2c59459ba73d2715cb1e615 Add autoconf checking messages for two checks As it seems that you have updated your repo, I have rebased the patch of the version from comment #16 to: e8529ff80715ce2206ccceebe07e8febf3539e46 Updated Serbian translation You have already reviewed that version of the patch, as you state in comment #17 After that, I re-applied the changes I made, and bundled them into another commit, which is included in the patch. If you need the un-rebased version of this second commit, ask me.
Hi Sinlu, Thank you for the updated patch in comment #24. This patch applies cleanly for me and contains 2 commits :-) I will begin testing and review. Regarding commit entries, it is desirable to have the first line of the commit message be less than 80 characters. This makes it easier to identify the messages in the commit log. I thought I would mention this because the second commit contains a very long first line in the commit message. We can amend this commit message later.
Hi Sinlu, When I tried to compile this patch set, it looks like we have some conflicts with the patches from Bug #688882 - Improve clearing of file system signatures. When I tried to compile with the patch set from comment #24 I receive the following error: <snip> GParted_Core.cc: In member function ‘bool GParted::GParted_Core::name_partition(const GParted::Partition&, GParted::OperationDetail&)’: GParted_Core.cc:2004:72: error: ‘open_device_and_disk’ was not declared in this scope GParted_Core.cc:2015:45: error: ‘close_device_and_disk’ was not declared in this scope make[2]: *** [GParted_Core.o] Error 1 make[2]: Leaving directory `/home/gedakc/workspace/gparted.dev2/src' make[1]: *** [all-recursive] Error 1 make[1]: Leaving directory `/home/gedakc/workspace/gparted.dev2' make: *** [all] Error 2 The problem is likely related to the following commit: Refactor and rename GParted_Core::open/close_device_and_disk() https://git.gnome.org/browse/gparted/commit/?id=e218ba3358eac27447b67dea6792987a30ac0a19 Would you be able to look into this problem?
Note that git will permit you to edit previous commits in your local repository with the following command: git rebase bbc643cd^ --interactive See, http://git-scm.com/book/en/Git-Tools-Rewriting-History#Changing-Multiple-Commit-Messages Normally we only edit previous commits that have not been committed to the GNOME git repository master branch. Once commits have been published then it is best to not edit them, but rather to make a new commit to make any necessary changes. Note that we have also updated the GParted git page instructions including how to test patches. http://gparted.org/git.php
Created attachment 240445 [details] [review] fixed rebase errors Hi Curtis, Thank you for your report about compiling errors, and your help with git. The commit message of the second commit wasn't created by me as one line. I didn't knew I have to place an empty line between the first line and the following body. I assume, that out of some reason git format-patch interprets the whole first paragraph as a summary, and removes all newlines from it. git log wasn't bothered by this issue, it showed separate lines all time, at least for the commit I created the patch from. Regarding Bug #688882, I've replaced the method names. While testing the result, it still didn't compile due to another rebase issue, so that I fixed it.
Hi Sinlu and Curtis, I had a look at this patch set and my biggest concern is that breaks git blame traceability of code because it copies Dialog_Partition_Info.cc to Dialog_Partition_Properties.cc and then deletes it in a separate patch. I guess that you will think it too big a job to refactor the patch set to git mv one file to the other making minimal changes in the first patch and adding extra features in subsequent patches so that git can track the code history. However if you want to do this I can help or even do it on your behalf. Thanks, Mike
Created attachment 240488 [details] [review] split up first commit to preserve git blame functionality Hi Mike, Thank you for looking at my patch and your good point. To preserve git blame traceability for Dialog_Partition_Properties class, I've moved the renaming into another commit. Greetings, Sinlu
Thank you Mike for assisting with this patch, and thank you Sinlu for making the changed suggested by Mike. The patch in comment #30 applies and compiles for me. The warnings on trailing whitespace in the file rename commit are to be expected since these exist in the file being renamed. This patch marks a significant change to GParted. As such I think it best to re-iterate why we think it is beneficial to make the change from a read-only partition information dialog to a read/write partition properties dialog. REASONS TO CREATE EDITABLE PARTITION PROPERTIES DIALOG 1) Request to add another label operation (GPT Partitions) Currently GParted supports volume labels, which are changed via the "Partition --> Label" menu. Sinlu has pointed out that GUID Partitions can also contain their own label. Unfortunately the current GParted names are not set up well to deal with a File System Label (also volume label), and a Partition Label (in GPT). Hence we would need to at least change the menu entries. Reference: Bug #690953 - Partition name support 2) Minimize label confusion using single properties dialog If we place both the File System (Volume) Label, and the Partition Label in a single dialog, then this will help distinguish between the two types of labels. We believe this will be less confusing to GParted users. 3) The "Partition" sub menu is growing quite large Rather than simply adding another sub menu to the Partition menu for setting Partition Labels, we have an opportunity to choose a different approach. More specifically we can migrate some menu item handling to a single "Properties" dialog. This change will remove the following menu entries under the Partition menu: Format to Label New UUID 4) The Re-Format operation does not support labelling The current "Partition --> Format to" menu only permits reformatting the file system, but not specifying a volume label at format time. While there are other ways to address this shortcoming (for example add a new format dialog), we can also fix this problem with a shared Properties Dialog. In addition to the above reasons, longer term we can investigate making "Manage Flags" into a queuable operation, and then placing this in the Partition Properties dialog. Currently "Manage Flags" immediately writes the changes in partition flags when exiting the dialog. Mike, What are your thoughts on this approach?
Thanks for the explanation Curtis. The approach from comment #31, points (1), (2) and (4) seem OK. Not sure about (3) ... I have now actually run the code and tested the new Partition Properties dialog. At the moment the new dialog is a UI disaster. Sorry. This is clouding my jugment whether a combined dialog with modifiable controls and displaying the old Partition Information is a good idea or not. Perhaps a separate dialog or just separate tabs for the editable bits from the display only bits would be better. Not sure at the moment. I tried looking through some system configuration applets and full applications on my GNOME 2 and XFCE desktops. None of them that I have seen combine modifiable controls and display information in the same dialog like the new Partition Properties does. They always have separate dialogs. If you see any other programs with a similar dialog to what is envisioned for Partition Properties please point it out. The current status of Partition Properties dialog is a UI disaster because: A1) Elements of the dialog change from what looks like fixed text into editable controls based on ticking the "Format file system" check box. This breaks the least supprise principle. It should be clear up front what is an edit control, grayed out if not currently usable. A2) Jumping between fixed text and editable controls resizes widgets moving them about so the "Format file system" check box moves from under the mouse is nasty. I also saw a few bugs during a quick testing of the dialog and a couple of minor coding issues but they are small items compared to getting the UI design right. Sorry, Mike
Thank you Mike for your honest opinion. Considerable time has already been spent developing the Partition Properties dialog. Having said that we definitely want to get the User Interface right. Generally we try to conform to the GNOME Human Interface Guidelines. If you have not been able to find a similar dialog to Partition Properties, then perhaps this is in violation of the GNOME HIG. It would be helpful if we could find a guideline that mentions this specifically. I have taken a look through the gtkmm coding examples. Unfortunately I have not been able to find an example similar to Partition Properties where we mix read-only and editable properties in the same dialog. The blame for the suggestion of having an editable Partition Properties dialog lies with me. Sinlu has been gracious enough to develop the code to implement the Partition Properties dialog. Mike and Sinlu, Before we proceed further I believe we need to identify and agree on what the correct path forward should be. As such I suggest we determine the best way to add Partition Labels to GParted before we update any of these patches. To get us started I think it best if we revisit the location where we tried to come up with a GUI design. The original discussion of how to handle the GUI changes for adding Partition Labels in addition to existing Volume Labels started in the following commment: https://bugzilla.gnome.org/show_bug.cgi?id=690953#c2 A new option (C) that I can think of would be: C) RENAME EXISTING LABEL TO "LABEL FILE SYSTEM" AND ADD "LABEL PARTITION" This does; however, increase the size of the sub menu under "Partition". Do you have other UI suggestions? I haven't given up on an editable Partition Properties dialog either. However, we do need to come to an agreement on how to add Partition Labels to the GParted GUI.
Thank you Mike for your opinion. I have found a such a mixed dialog for a program with a very similar task to gparted's (it's not Gnome, but does that matter?): http://blog.volker-lanz.de/2010/09/01/release-kde-partition-manager-live-cd-1-0-3 please focus your attention to the second image. Such a mixed approach is also very common for file system property panels, so that it even made into the gnome HID: http://developer.gnome.org/hig-book/2.32/windows-utility.html.en#property-windows http://www.techotopia.com/index.php/Browsing_My_Computer,_Files_and_Folders_on_the_Ubuntu_10.x_Desktop#Changing_File_and_Folder_Permissions So we wouldn't be the first to implement a mixed dialog. Your two remarks A1) and A2) are both on the same feature of the program. I agree that this conversion between label and dropdown box violates the least supprise principle. You already suggest that the dropdown box can be greyed out so that the user cannot edit. This is a good approach to solve that issue. Hi Curtis, This new Option C) sounds not bad, but I like the property window approach more, because, with the partition window, when editing multiple properties at once, you have to do less mouse clicks to do the same thing. Imagine you want to edit n properties of a partition. Lets view the old method: For each property to change do open partition menu (or right-click onto the partition) click onto the property to change change the property press enter or click on OK done So we have 3n clicks. Of course you can open the partition menu by hovering, but you have to move your mouse to do this. And the partition properties method: open partition menu (or right-click onto the partition) click onto "properties" For each property to change do enable the property to change change the property done press enter or click on OK with this approach we have n + 3 clicks.
Thank you Sinlu for finding those pictures. I actually downloaded the KDE Partition Manager LiveCD and tried it in a virtual machine. (Get it from: http://sourceforge.net/projects/partitionman/) After seeing it in action I think that a dialog with mixed control and information display can work. A few points: B1) One thing which I think helps KDE P.M. is that the New and Properties dialogs feel to be laid out similarly. B2) Putting as little information as is absolutely required into the dialog seems to help. Perhaps use tabs or twizzlers (those things used to hind lines in the operation window) to hide extra details. Just an idea. I have lots of further ideas swimming around in my head about how the dialog should work. Sinlu, as this is your project I don't want to step on your toes. If you want some suggestions or to even work together on the UI design let me know. (I've only done a UI design once before but that wasn't GTK/GNOME so I don't really know all their guidance. Plus that started in the same room with everyone, using a white board drawing what it could look like). Mike
Hello Mike, Could you please better explain what you mean with twizzlers? B1) I agrree that it helps KDE PM that its new and properties dialogs are similar. So are gparted's resize/move and properties dialogs. The new partition dialog would be similar to both too, but unfortunately the entries are in two columns there. Perhaps in this case this makes more sense, because the visualisation at the top is editable and therefore broader. Not using up the horizontal space wouldn't look nice. A two-column approach for the gparted properties window is not the best solution as I think Gnome HID rules discourage the use of that. Correct me when I'm wrong. https://developer.gnome.org/hig-book/2.32/design-window.html.en B2) The number of entries in the properties window is large in comparison to property windows of other applications. It has 13 entries and two additional checkboxes. That's quite a lot. One possibility I see to split up would be in filesystem and partition categories as it is now, just with tabs. Note that every additional control makes the UI more complicated, which it has to repay first by its improvements in simplicity. Curtis and Mike Do you know applications that have two and not more Tabs? I'm not sure whether this is discouraged or not. Thank you Mike for your offer to help. I think, like Curtis, that all of us should agree on the best way to do the UI. I welcome your suggestions and constructive criticism. So if you have any ideas about the properties window, just name them. An open discussion can only improve things. So, I'm interested in these lots of ideas you speak about. ;-) Greetings, Sinlu
Hi Sinlu and Mike, It is good to see some constructive discussion on this idea, and it sounds like we can make the Dialog Properties window work and still follow the GNOME HIG. :-) Regarding B2) in comment #35, if by "twizzlers" you mean "lines", then lines are deprecated for use to separate GUI items. The guideline I am referring to is the following: Using frames with visible borders to separate groups within a window is deprecated. Use spacing and bold headers instead. This is more effective because there are fewer gratuitous lines to distract the user from the main content in the window. See Section 6.19 ― Frames and Separators for more details. See, https://developer.gnome.org/hig-book/2.32/design-window.html.en#layout-dialogs Regarding B1) in comment #36, I think that we can indeed have two columns. One item support this is at the bottom of the design-window link that states: Do not design windows that are more than 50% longer in one dimension than in the other. People are more comfortable looking at windows and dialogs whose dimensions stay within the golden ratio (about 1.6 to 1), a ratio that artists and architects have used to create aesthetically-pleasing paintings and buildings for thousands of years. Hence having a window wider than it is long might be advantageous for the amount of information we have to display. Regarding B2) in comment #36, I think that we can still get all items of information in a single dialog without tabs. I just recently read another HIG rule that might help with the layout of the Properties Dialog. Specifically, Arrange controls in your dialog in the direction that people read. In western locales, this is generally left-to-right, top-to-bottom. Position the main controls with which the user will interact as close to the upper left corner as possible. Follow similar guidelines for arranging controls within groups in the dialog, and for specifying the order in which controls are traversed using the Tab key. See, https://developer.gnome.org/hig-book/2.32/windows-dialog.html.en#dialog-layout Using this as a guide, we could have two columns with editable properties on the left and read-only properties on the right. Following is a sample design. Please feel free to propose alternate designs or suggest changes to this design. --------------- begin sample layout --------------- Properties of /dev/sdd1 *File System* Label: ________________________ Used: ##.## MiB (0%) File system: <NTFS> Unused: ##.## GiB (100%) [*] Reformat file system Size: ##.## GiB UUID: XXXX-XXXX-XXXX-XXXX [*] Change UUID {*LVM2 PV*} Volume group: xxxxxx Size: ##### MiB Members: xxxx, xxxx *Partition* Label: ________________________ First sector: #### Path: /dev/sdd1 Last sector: ########## Flags: xxxx Total sectors: ########## {*Warning*} Optional warning text that would span across both columns.... --------------- end sample layout --------------- Where: < > indicates a drop down list [ ] indicates a checkbox { } indicates optionally displayed information With the above design, I placed the labels at the top of the respective sections because I think these are the most likely items that a user will change. The tab order would be set to follow the way a person reads newspaper columns. Namely, starting at the top of the left editable properties and progressing to the bottom. Then starting again at the top of the read-only properties in the right column and progressing to the bottom. If you think that this is too much information, then we might need to think of a tabbed approach, or some other design.
By "twizzlers" I mean the hide and expand widget at the bottom of the File System Support dialog by the text "Legend". Don't what it's really called. Anyway it was only an idea and possibly a bad one. Curtis' design looks pretty good. Few suggestions: 1) Remove Size from the LVM2 PV section. (It's actually the file system size but it's been split away from Used & Unused by LVM2 PV section in Partition Properties). 2) I think that the Partition label should be left out for now and added later with Bug 690953 "Partition name support". Move Partition and Flags up one line. 3) I think that the buttons at the bottom should be, when no change has been requested: [ / Apply ] [ X Close ] (Apply button will be disabled). When a change has been entered the buttons should be: [ / Apply ] [ O Cancel ]
Hi Curtis, Thank you for your detailed further thoughts about the two-column model. My remarks on them: 1) To explain my original declination of the two-column-approach: In https://developer.gnome.org/hig-book/2.32/design-window.html.en#layout-dialogs they write something about anchors for eye scanning: As the labels are all similar in length, they should be left-aligned. Now the user has a firm left margin to anchor the eye and scan the list of items vertically more easily. If most of the labels in a group greatly differ in length, right-align them instead, so that the controls do not end up too far away from their corresponding labels. My thought was, that the two-column-model would disrupt a straight and linear eye scan path from the left top to the left bottom, where the labels are. In the two-column approach, the user would have to scan the right side, too, and it would be more difficult for him to associate the entries into a group like filesys, lvm2 pv or partition. 2) With the two-column-approach, the Property dialog for unallocated space would look like this: --------------- begin sample layout --------------- Properties of unallocated File system: unallocated *Partition* Size: ##.## GiB First sector: #### Last sector: ########## Total sectors: ########## --------------- end sample layout --------------- I think you agree that it is better to have more content on the left as on the right, and not the other way round as its here. (unallocated specific) Rearrangements could perhaps break the least suprise principle. Such a rearrengement would be: --------------- begin sample layout --------------- Properties of unallocated File system: unallocated Size: ##.## GiB *Partition* First sector: #### Last sector: ########## Total sectors: ########## --------------- end sample layout --------------- Note, that we already rearranged by moving the Size property one category down, as the nonexistent filesystem of unallocated space has no size. 3) #Warning texts# Warnings should span both columns, especially when they are longer, as too much wrap on one side and unused space on the other side doesn't look good. On the other hand, making warning texts span both columns would create a breakage to the two-columned part of the dialog, so perhaps it would be more apropriate to place them at the top, right below the partition visualisation. 4) Curtis you rise a good point about placing the labels at the top. This should be done, even if we decide against the two columns. 5) The one column properties window has the dimensions of, with enabled dropdown box, 524x431 for me, which is within the ratio of 1.2 to 1. So the dialog is not too long for the golden ratio, it is too short. Rough calculations of mine lead to the dimensions of the two column properties window of 414x731, which is 1.8 to 1, so closer to the golden ratio. 6) Warnings can be displayed more safely when the window is not so long. 7) KDE PM's property window has 13 properties and one checkbox, similar to current property window. Note that KDE PM's property window has one column. But I think I could agree onto a two-column design, too. Hi Mike, ah I understand now what you meant by twizzlers. Tank you. 2) Note that Partition label is already implemented and working (see commit message of second commit in the patch set to this bug). This would need to split up the second commit of my patch set, but this time it would be not so easy. Don't know if its worth the time. 3) Thank you for this idea. It might violate the least suprise principle, and it isn't in use for the property dialogs I know. Name me one when you do.
Hi Sinlu and Mike, Based on your comments I have drafted two different proposals for the Partition Properties that attempt to incorporate your suggestions. I made the following changes based on your feedback: i) Removed Size from LVM2 PV section ii) Moved Size from File System section to Partition Section In terms of aesthetics, I think I lean towards the one column display, especially when I think about the "unallocated" example shown in point (2) of comment #39. The one column display is long. The question I ponder is whether it is too long. If you have other proposals, then this is the time to get them on the table so we can pick/build the best overall solution. Curtis A) TWO COLUMN PROPOSAL ---------------------------------------------------------------------- Properties of /dev/sdd1 *File System* Label: ________________________ Used: ##.## MiB (0%) File system: <NTFS> Unused: ##.## GiB (100%) [*] Reformat file system UUID: XXXX-XXXX-XXXX-XXXX [*] Change UUID {*LVM2 PV*} Volume group: xxxxxx Members: xxxx, xxxx *Partition* Label: ________________________ First sector: #### Path: /dev/sdd1 Last sector: ########## Flags: xxxx Total sectors: ########## Size: ##.## GiB {*Warning*} Optional warning text that would span across both columns.... ---------------------------------------------------------------------- Where: < > indicates a drop down list [ ] indicates a checkbox { } indicates optionally displayed information B) ONE COLUMN PROPOSAL ---------------------------------------------------------------------- Properties of /dev/sdd1 *File System* Label: ________________________ File system: <NTFS> [*] Reformat file system UUID: XXXX-XXXX-XXXX-XXXX [*] Change UUID Used: ##.## MiB (0%) Unused: ##.## GiB (100%) {*LVM2 PV*} Volume group: xxxxxx Members: xxxx, xxxx *Partition* Label: ________________________ Path: /dev/sdd1 Flags: xxxx First sector: #### Last sector: ########## Total sectors: ########## Size: ##.## GiB {*Warning*} Optional warning text.... ----------------------------------------------------------------------
For Curtis' ONE COLUMN PROPOSAL: This will be even longer than the current Partition Information dialog. On my netbook with only a 600 pixel high screen, an LVM PV2 partition with a warning is already at the limit of what fits on the screen. Any more and I can't get to the [Close] button. If we use this we would need some UI widget to reduce the height, such as tabs. Example of changing labels Sinlu asked for, for my suggestion(3) from comment '38 about buttons: Apply an operation in GParted that takes a long time. You get a [Cancel] button. Click it and it changes into a disabled [Force cancel (5)] button, which becomes enabled after a 5 second count down, assuming the task doesn't cancel normally within 5 seconds. With my proposal for buttons the right most button is always the do nothing, close the dialog button, but just changes it label between [Cancel] and [Close] depending whether there are any pending changes or not. The [Apply] button only becomes active when there are pending changes in the dialog to apply. If we don't like the labels changing we should stick with [Cancel] as the right most, close the dialog and do nothing button.
For the height argument made by Mike: Note that the warning is to be filled into a scrollable, still consuming space but less. See comment #18 Point 3) and Bug #690542. The height issue rises the question about which minimum screen resolution we want to require, and I think the application should run smoothly with 1024x600. B1) ONE COLUMN PROPOSAL - resizable Perhaps the one-columned dialog can be made resizable, and scrollable. ADDING NEW ENTRIES Having a model that is on the borders of screen height would lead to the result, that it would be difficult to add new features (for example GPT Partition UUIDs). There might be a time when adding an entry means restructuring the partition properties window. The two column approach has the disadvantage that it needs to be rethought every time a new entry is added, and this in all its reorderings. Every time there would be a restructuring, so the users would have to get re-accustomed with every new feature. The best way for users and developers to add new entries, is with the tab model. On the other hand, a fast overview and fast possibility to change is not made easy by the tab model, this is my biggest concern against the model. But for adding entries I think its the best. C) TAB PROPOSAL This proposal leaves us with the question about where to place the warnings, the bottom is not the right place I think. ---------------------------------------------------------------------- Properties of /dev/sdd1 *File System* | {LVM2 PV} | Partition Label: ________________________ File system: <NTFS> [*] Reformat file system UUID: XXXX-XXXX-XXXX-XXXX [*] Change UUID Used: ##.## MiB (0%) Unused: ##.## GiB (100%) {*Warning*} Optional warning text.... ---------------------------------------------------------------------- ---------------------------------------------------------------------- Properties of /dev/sdd1 {*Warning*} Optional warning text.... File System | {*LVM2 PV*} | Partition Volume group: xxxxxx Members: xxxx, xxxx ---------------------------------------------------------------------- ---------------------------------------------------------------------- Properties of /dev/sdd1 {*Warning*} Optional warning text.... File System | {LVM2 PV} | *Partition* Label: ________________________ Path: /dev/sdd1 Flags: xxxx First sector: #### Last sector: ########## Total sectors: ########## Size: ##.## GiB ---------------------------------------------------------------------- Where: < > indicates a drop down list [ ] indicates a checkbox { } indicates optionally displayed information | indicates a separator between tabs, ** marks the current active tab Hi Mike, Thank you for the example from comment #41. I see some difference between the two cases. The button in the operation window changes when interacting with it directly. You propose, that the buttons in the properties window change when interacting with different controls. In my opinion, this is violation of the least suprise principle, while the operation window does not violate. Originally I made the order of the buttons the way you propose it in comment #41, but Curtis was against, see comment #4 point 4 and comment #5: For consistency across dialogs I still prefer to use the order "Cancel" followed by "Apply". I don't know which order is the best, I'd support both.
Hi Mike and Sinlu, When trying to answer the "right most button" proposal Mike raised in comment #41, I discovered that this is a larger issue. As such I have created the following report to continue the button name and button placement discussion: Bug #697449 - Improve Button Name and Button Placement Consistency I will respond to the other questions and proposals shortly. Curtis
MINIMUM TARGET SCREEN RESOLUTION PROPOSAL ========================================= Both Mike and Sinlu raise a good points about the limitations imposed by a 600 pixel high screen. I would like to propose we use a 600 pixel screen height as the minimum height we should design for in GParted. Further with the GParted main window defaulting to 775x500, and with common screen ratios of 16:10, 16:9, and 4:3, I propose the minimum width we should design for as 800 pixes. I will add this to the development guidelines if we are in agreement. Q1) Shall we make an 800x600 screen the minimum target screen resolution? SUMMARY OF PARTITION PROPERTIES/INFORMATION OPTIONS =================================================== In an effort to help us choose I have tried to summarize the pros and cons of the different options we are considering: A) TWO COLUMN WINDOW PROS: - All information is easily seen on 800x600 monitor - Likely no need for scrolling, other than warning message CONS: - More difficult to add / restructure for new information - anchors for eye scanning are not as good as one column display B) ONE COLUMN WINDOW PROS: - Easiest to add new information - All information is easily seen on a large enough monitor - Good anchors for eye scanning CONS: - 600 pixel height means scrolling on small screens - will hit practical limit of viewable information soonest C) TABBED ONE COLUMN WINDOW PROS: - Less real-estate concerns because more tabs can be added - Good anchors for eye scanning CONS: - All information is not seen at once, must navigate tabs - More code complexity to add new tabs and information Looking through the above options, I do prefer the one column approach, but if we design for a minimum 600 pixel height then we probably have to seriously consider the tabbed approach. With the tabbed approach in mind I have the following suggested change to Sinlu's tabbed proposal in comment #42: i) Add "Size: ##.## GiB" back to *File System* tab. This will help alleviate the need to switch to other tabs trying to find the size. We could also consider adding "Size: ##.## GiB" to the *LVM2 PV* tab.
(In reply to comment #44) > MINIMUM TARGET SCREEN RESOLUTION PROPOSAL > ========================================= ... > I will add this to the development guidelines if we are in agreement. > > Q1) Shall we make an 800x600 screen the minimum target screen > resolution? Agreed. Sounds good to me. > SUMMARY OF PARTITION PROPERTIES/INFORMATION OPTIONS > =================================================== ... > A) TWO COLUMN WINDOW > B) ONE COLUMN WINDOW > C) TABBED ONE COLUMN WINDOW ... > Looking through the above options, I do prefer the one column > approach, but if we design for a minimum 600 pixel height then we > probably have to seriously consider the tabbed approach. Based on how it looks as plain text in this bug in comments #40 and #42 I prefer option A) TWO COLUMN WINDOW. To me the two column window scans well to the eye with attributes on the left and sizes on the right. It's also the layout which hides the least information with tabs and scrolling on smaller screens. With correct use of box widgets to layout display elements it should be no harder to add new items than the other options. To be able to make a more informed decision the options would need mocking up in a paint or drawing program so we can see what they really look like. > With the tabbed approach in mind I have the following suggested change > to Sinlu's tabbed proposal in comment #42: > > i) Add "Size: ##.## GiB" back to *File System* tab. > > This will help alleviate the need to switch to other tabs trying > to find the size. > > We could also consider adding "Size: ##.## GiB" to the *LVM2 PV* > tab. I would say all options should have the "Size: ##.## GiB" back with the Used and Unused (and optionally Unallocated) figures in the *File System* section as they it displays like a total. Used + Unused + Unallocated -> Size. Used: ##.## GiB (22%) Unused: ##.## GiB (75%) Unallocated: ##.## GiB (3%) Size: ##.## GiB (100%)
(In reply to comment #45) > (In reply to comment #44) > > SUMMARY OF PARTITION PROPERTIES/INFORMATION OPTIONS > > =================================================== > ... > > A) TWO COLUMN WINDOW > > B) ONE COLUMN WINDOW > > C) TABBED ONE COLUMN WINDOW > ... > > Looking through the above options, I do prefer the one column > > approach, but if we design for a minimum 600 pixel height then we > > probably have to seriously consider the tabbed approach. > > Based on how it looks as plain text in this bug in comments #40 and #42 > I prefer option A) TWO COLUMN WINDOW. An advantage to option A) TWO COLUMN WINDOW is that all of the information is readily viewable on one screen. This is a big plus versus having to switch among tabs. Can I change my preference to option A) TWO COLUMN WINDOW? :-) Sinlu, Could you work with option A) TWO COLUMN WINDOW? If so then we should draft another text mock-up to try to incorporate all of the points we have discussed so far.
Hi Curtis and Mike, I can live with option A) TWO COLUMN WINDOW, too. :-) A also agree to Q1) from comment #44: > Q1) Shall we make an 800x600 screen the minimum target screen > resolution? Others have come to a similar conclusion: http://techbase.kde.org/Projects/Usability/HIG/Dialogs#Dialog_Layout Greetings, Sinlu
Hi Sinlu and Mike, I have added the 800x600 target minimum screen resolution to the GParted development page. See, http://gparted.org/development.php Following is an updated two column proposal in which I try to incorporate your suggestions. For now we can simply use [Apply] and [Cancel] buttons at the bottom of the screen, and [Close] if read-only. We have a separate bug report to deal with button inconsistency. See, Bug #697449 - Improve Button Name and Button Placement Consistency Have we missed anything, or should we proceed with development again? Regards, Curtis A) TWO COLUMN PROPOSAL ---------------------------------------------------------------------- Properties of /dev/sdd1 *File System* Label: ________________________ Used: ##.## MiB (0%) File system: <NTFS> Unused: ##.## GiB (100%) [*] Reformat file system {Unallocated:} ##.## MiB (#%) UUID: XXXX-XXXX-XXXX-XXXX Size: ##.## GiB [*] Change UUID {*LVM2 PV*} Volume group: xxxxxx Size: ##.## GiB Members: xxxx, xxxx *Partition* Label: ________________________ Size: ##.## GiB Path: /dev/sdd1 Flags: xxxx First sector: #### Last sector: ########## Total sectors: ########## {*Warning*} Optional warning text that would span across both columns.... Apply Cancel ---------------------------------------------------------------------- Where: < > indicates a drop down list [ ] indicates a checkbox { } indicates optionally displayed information Please note that I would also like to add to the *Partition* section another "Size in MiB:" value under "Size:" for people trying to recreate a partition size exactly (because GParted only supports specifying MiB sizes).
I don't think that we need "Size: ##.## GiB" in the *LVM2 PV* section. Would it be OK to have the sector figures in the *Partition* section in the right hand column. It would use less height and keep all the figures in the right hand column. A) TWO COLUMN PROPOSAL ---------------------------------------------------------------------- Properties of /dev/sdd1 *File System* Label: ________________________ Used: ##.## MiB (0%) File system: <NTFS> Unused: ##.## GiB (100%) [*] Reformat file system {Unallocated:} ##.## MiB (#%) UUID: XXXX-XXXX-XXXX-XXXX Size: ##.## GiB [*] Change UUID {*LVM2 PV*} Volume group: xxxxxx Members: xxxx, xxxx *Partition* Label: ________________________ First sector: #### Path: /dev/sdd1 Last sector: ######## Flags: xxxx Total sectors: ######## Size: ##.## GiB {*Warning*} Optional warning text that would span across both columns.... Apply Cancel ---------------------------------------------------------------------- Where: < > indicates a drop down list [ ] indicates a checkbox { } indicates optionally displayed information
Would it be OK to display the partition size after the sectors like below. It keeps all the figures in a column of the same units, namely sectors. *Partition* Label: ______________________ First sector: #### Path: /dev/sdd1 Last sector: ######## Flags: xxxx Total sectors: ######## (##.## GiB)
> I don't think that we need "Size: ##.## GiB" in the *LVM2 PV* > section. We can remove "Size: ##.## GiB" from the *LVM2 PV* section. I had added it there because I thought you (Mike) wanted "Size:" in every section. My misunderstanding. > Would it be OK to have the sector figures in the *Partition* section > in the right hand column. It would use less height and keep all the > figures in the right hand column. The concern with the sectors being in the second column was mentioned by Sinlu in comment #39 point (2). Specifically in the "Unallocated" case, there is no "Label:", "Path:", or "Flags:". I think that we should be okay with your suggestion. It might look a bit strange though with nothing in the first column of the *Partition* section. Alternatively we could handle the special case of unallocated space and shift column 2 to column 1. This might break the principle of least surprise though. > Would it be OK to display the partition size after the sectors like > below. It keeps all the figures in a column of the same units, namely > sectors. I would be okay with this assuming there is enough room. There should be on an 800 pixel wide screen.
(In reply to comment #51) > > Would it be OK to have the sector figures in the *Partition* section > > in the right hand column. It would use less height and keep all the > > figures in the right hand column. > > The concern with the sectors being in the second column was mentioned by Sinlu > in comment #39 point (2). Specifically in the "Unallocated" case, there is no > "Label:", "Path:", or "Flags:". > > I think that we should be okay with your suggestion. It might look a bit > strange though with nothing in the first column of the *Partition* section. > > Alternatively we could handle the special case of unallocated space and shift > column 2 to column 1. This might break the principle of least surprise though. I know the current Partition Information dialog doesn't display a path for the unallocated partition case, but as the main window reports a path of unallocated the new Partition Properties could too. Like this: *Partition* Path: unallocated First sector: #### Last sector: ######## Total sectors: ######## (##.## GiB)
Hi Curtis and Mike, In reply to comment #48: >I have added the 800x600 target minimum screen resolution to the >GParted development page. See, >http://gparted.org/development.php Thank you Curtis for adding the screen resolution requirement to the development page. In reply to comment #52: >I know the current Partition Information dialog doesn't display a path >for the unallocated partition case, but as the main window reports a >path of unallocated the new Partition Properties could too. Good idea.
Unless there are any other suggestions, I think we are good to go with developing the code for this improvement.
Created attachment 241418 [details] [review] Split the window into two columns Hi Curtis and Mike, I've reordered the elements.
Hi Sinlu, 'Just wanted to say that we didn't miss your patch update. Currently we are busy investigating a crash in GParted. See: Bug #697727 - Segfault in livecd Gparted v 0.15.0-3 when copying partition
Hi Curtis, thank you for informing me.
Created attachment 243467 [details] [review] Fix two whitespace errors Hi, just fixed whitespace errors introduced in my last commit.
Thank you Sinlu for your patience while we worked to resolve some serious gparted crash problems. Your updated patch from comment #58 captures many of the GUI changes we discussed. :-) Some minor items: 1) When displaying an unallocated portion of disk, the "File System" section title is missing above the line "File system: unallocated" 2) The "UUID" field label under the "File System" section is missing a colon. For example "UUID:" 3) A new reformat option "cleared" is available since GParted 0.16.0. This new option should be placed at the end of the list of file systems similar to how this works in 0.16.0 and 0.16.1. 4) If possible more spacing between the two columns might make the dialog more pleasing to the eye. This would push field labels like "Used:", "Unused:", and "Size:" further to the right. When we get the GUI and functionality working the way we want, then we can move on class and method naming consistency in the code and cleaning up the no longer needed code.
Created attachment 243823 [details] [review] Changes after suggestions of comment #59 Hi Curtis, I've created a new patch, adding commits without amending already posted ones. 1) Like the File System-field, the headline gets now displayed all time. 2) Could you give me a specific line of code please? I can't find the missing colon neither in code nor in UI, only I see the line: table ->attach( * Utils::mk_label( "<b>" + Glib::ustring( _("UUID:") ) + "</b>"), Did you mean the "Change _UUID" checkbox? This doesn't need a colon, does it? Perhaps this comes from i18n and en_CA.po, where "UUID:" is mapped to "UUID". Does the issue remain when invoking: # LANGUAGE=en_US:en,LANG=en_US.UTF-8 gksudo src/gpartedbin ? 3) Wouldn't it be more senseful to modify the option order in the FILESYSTEM enum? Changing the order in the interface code creates redundancies. To ensure that cleared is last, a comment could be added to the enum, and to the GParted_Core::find_supported_filesystems() method about to conceive the enum order. 4) How much space do you think would please the eye most? To test spaces, you can add the line: table ->set_col_spacing( 2, <test_space> ) ; to the end of Dialog_Partition_Properties::Display_Info() method. Greetings Sinlu
Hi Sinlu, Thank you for the updated patch. Response to suggested changes follow: 1) Confirmed fixed. :-) 2) My bad. This is an old problem with the en_CA translation. See: Bug #685735 - GParted - Add missing colon to label in en_CA translation No change required. 3) We currently have special handling for the types that are not actual file systems. For example: FS_UNFORMATTED, FS_EXTENDED. Since FS_CLEARED is not an actual file system it might be better to keep this as a special case. Mike, We would appreciate your thoughts on this issue regarding FS_CLEARED special handling versus later placement in Utils.h enum list. 4) Your tip for testing the spacing works great. Personally I like a spacing of 24 pixels for the second column. I believe this helps make the size column more distinct from the column with the labels. Also a choice of 24 is exactly double the normal 12 spacing between a field label and the value. 5) Extra space patch in comment #60. When I applied the patch I received the following message: $ git am bug691681-partition-properties.patchApplying: <snip> Applying: Display filesystem headline all time (#691681) Applying: Move "cleared" entry to the bottom (#691681) /home/user/gparted/.git/rebase-apply/patch:40: space before tab in indent. if ( fstypes_optionmenu ->get_history() < FILESYSTEMS .size() ) warning: 1 line adds whitespace errors. $ 6) Warning message about comparison between signed and unsigned integers. When I compiled the code I received the following message: $ make <snip> g++ -DHAVE_CONFIG_H -I. -I.. -pthread -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -pthread -I/usr/include/gtkmm-2.4 -I/usr/lib/x86_64-linux-gnu/gtkmm-2.4/include -I/usr/include/atkmm-1.6 -I/usr/include/giomm-2.4 -I/usr/lib/x86_64-linux-gnu/giomm-2.4/include -I/usr/include/pangomm-1.4 -I/usr/lib/x86_64-linux-gnu/pangomm-1.4/include -I/usr/include/gtk-2.0 -I/usr/include/gtk-unix-print-2.0 -I/usr/include/gdkmm-2.4 -I/usr/lib/x86_64-linux-gnu/gdkmm-2.4/include -I/usr/include/atk-1.0 -I/usr/include/glibmm-2.4 -I/usr/lib/x86_64-linux-gnu/glibmm-2.4/include -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/sigc++-2.0 -I/usr/lib/x86_64-linux-gnu/sigc++-2.0/include -I/usr/include/cairomm-1.0 -I/usr/lib/x86_64-linux-gnu/cairomm-1.0/include -I/usr/include/pango-1.0 -I/usr/include/cairo -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng12 -I/usr/lib/x86_64-linux-gnu/gtk-2.0/include -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/gio-unix-2.0/ -DGPARTED_DATADIR=\""/usr/local/share"\" -DGNOME_ICONDIR=\""/usr/local/share/pixmaps"\" -DGNOMELOCALEDIR=\""/usr/local/share/locale"\" -Wall -g -O2 -MT Dialog_Partition_Properties.o -MD -MP -MF .deps/Dialog_Partition_Properties.Tpo -c -o Dialog_Partition_Properties.o Dialog_Partition_Properties.cc Dialog_Partition_Properties.cc: In member function ‘GParted::FILESYSTEM GParted::Dialog_Partition_Properties::get_filesystem()’: Dialog_Partition_Properties.cc:763:64: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] mv -f .deps/Dialog_Partition_Properties.Tpo .deps/Dialog_Partition_Properties.Po
One more item I just noticed. 7) When Reformat file system is enabled, and I click on the File system drop-down list, the FS color boxes are missing to the left of each of the file systems. For example, see the color boxes beside the "Format to" menu option with a partition selected.
Created attachment 243954 [details] [review] Add FS color boxes to partition properties, two minor issues Hi Curtis Thank you for reviewing my patch and your patience with my whitespace errors :-) Amended the last commit, and added one. 4) What about 60? Its 12*5 and I get a close value to the golden ratio being 1,62 as the ratio of window height and width. But I think 24 would be OK, too. 5), 6) Thank you for noting these issues. I've amended the commit: Move "cleared" entry to the bottom (#691681) to fix them. 7) The color squares should appear now when expanding the drop-down list.
(In reply to comment #61) > 3) We currently have special handling for the types that are not actual > file systems. For example: FS_UNFORMATTED, FS_EXTENDED. Since > FS_CLEARED is not an actual file system it might be better to keep this > as a special case. > > > Mike, > > We would appreciate your thoughts on this issue regarding FS_CLEARED > special handling versus later placement in Utils.h enum list. I have had a quick test of the code from comment #63. Looks like Sinlu resolved the question over FS_CLEARED. This is not a full test, but just what I found on a quick test: 7) Coloured boxes still don't appear in the file system format drop down. 8) Ticking both "Reformat file system" and "Change UUID" only results in one pending operation being added. The format as FS operation is added, but not the new random UUID operation. 9) Display of a partition with unallocated file system space is not correct. The file system colour boarder isn't complete. Looks like the size of the FS colour rectangle has been limited to the size of used+free+4 pxls not the full width of the box. 10) Animated opening of pending operations happens before the dialog is closed. For every other dialog the animated opening happens after the dialog is closed. Check new or label. 11) Partition properties offers to set/change partition label on a MSDOS partition. a) Reformat an existing partition and add to pending operations. b) Re-open partition properties. It offers to change the partition label when it shouldn't be possible. Mike
Thank you Mike for your comments. Following is one quick observation. (In reply to comment #64) > 8) Ticking both "Reformat file system" and "Change UUID" only results > in one pending operation being added. The format as FS operation is > added, but not the new random UUID operation. When a partition is reformatted with GParted (prior to this patch), I believe it receives a new UUID by default. As such an extra step to set a new random UUID should not be necessary.
Created attachment 244262 [details] [review] fixed issues 10 and 11 of comment 64 Hi Mike, thank you for testing my patch. 7) Currently colored boxes don't appear in the closed drop down element, only when its expanded. I think its better not to display the color boxes in the closed drop down, what do you think? 8) As Curtis already pointed out, a partition gets a new UUID when being reformatted, so there is no effective need for two operations. Should the UUID checkbox become checked and then disabled when ticking the reformat operation, or does this violate the least suprise principle? 9) Can't follow your point. For me unallocated space is just displayed as a (labeled) grey rectangle without any border. Is this a regression by my patch? Except of replacing "Dialog_Partition_Info" with "Dialog_Partition_Properties", I didn't find any changes of the init_drawingarea method which is responsible for partition drawing. 10, 11) good points. Fixed them in two additional commits to my patchset.
Created attachment 244317 [details] Screenshot panorama Hi Sinlu, Just tried your latest patch set from comment #66. 7) Coloured boxes don't appear in the File system format dropdown in Debian 6 and Fedora 14. They do in Fedora 18. (In attached panorama see screenshot #1 Debian 6 without coloured boxes, #2 Fedora 18 with coloured boxes). 8) Not adding a new UUID operation is OK when formatting as a new file system as per Curtis' reasoning from comment #65. 9) The case is when a file system doesn't fill the partition and GParted is displaying unallocated space within the partition. (In attached panorama see screenshot #3. Compare blue surround in partition properties top and the older partition information below). 10 & 11) Tested and passed. 12) Using the existing "Partition > Format to" to reformat an existing partition produces the following error dialog. Is this intended or a bug? (-) Cannot format this file system to FSTYPE A FSTYPE file system requires a partition of at least 0.00 B. 13) The second patch doesn't compile on CentOS 5.9. End of the make output: if g++ -DHAVE_CONFIG_H -I. -I. -I.. -pthread -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/gtkmm-2.4 -I/usr/lib/gtkmm-2.4/include -I/usr/include/glibmm-2.4 -I/usr/lib/glibmm-2.4/include -I/usr/include/gdkmm-2.4 -I/usr/lib/gdkmm-2.4/include -I/usr/include/pangomm-1.4 -I/usr/include/atkmm-1.6 -I/usr/include/gtk-2.0 -I/usr/include/sigc++-2.0 -I/usr/lib/sigc++-2.0/include -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/lib/gtk-2.0/include -I/usr/include/cairomm-1.0 -I/usr/include/pango-1.0 -I/usr/include/cairo -I/usr/include/freetype2 -I/usr/include/libpng12 -I/usr/include/atk-1.0 -DGPARTED_DATADIR=\""/usr/share"\" -DGNOME_ICONDIR=\""/usr/share/pixmaps"\" -DGNOMELOCALEDIR=\""/usr/share/locale"\" -Wall -g -O2 -MT Dialog_Partition_Properties.o -MD -MP -MF ".deps/Dialog_Partition_Properties.Tpo" -c -o Dialog_Partition_Properties.o Dialog_Partition_Properties.cc; \ then mv -f ".deps/Dialog_Partition_Properties.Tpo" ".deps/Dialog_Partition_Properties.Po"; else rm -f ".deps/Dialog_Partition_Properties.Tpo"; exit 1; fi Dialog_Partition_Properties.cc: In member function ‘void GParted::Dialog_Partition_Properties::filesystem_optionmenu_changed()’: Dialog_Partition_Properties.cc:301: error: ‘class Gtk::Entry’ has no member named ‘set_visible’ Dialog_Partition_Properties.cc:309: error: ‘class Gtk::Entry’ has no member named ‘set_visible’ make[2]: *** [Dialog_Partition_Properties.o] Error 1 make[2]: Leaving directory `/home/centos/programming/c/gparted-testbuild/src' Thanks, Mike
Created attachment 244341 [details] Merged properties screenshot with 60 and 24 pixel spacing of second colum Hi Sinlu, I have also been testing the patch in comment #66. Following are my observations. 3) I can confirm that the "cleared" option is now available in the list of file systems when "Reformat file system" is enabled. The message that appears in the queued operation pane is: "Format /dev-path as cleared with label "MyLabel". With "cleared", no file system label is written so we probably need some special handling for to blank the Label and for the queued operation message in the case of "cleared". > 4) What about 60? Its 12*5 and I get a close value to the golden > ratio being 1,62 as the ratio of window height and width. But I > think 24 would be OK, too. I have attached a screenshot with both 60 pixel and 24 pixel spacing. My preference is for something less than 60 pixel spacing. Mike, Do you have a preference? 5) Confirmed extra white-space message fixed. 6) Confirmed warning message about integer comparison fixed. 7) Colored squares beside file system testing results: Do appear in: Kubuntu 12.04 Debian 6 (this VM includes up-to-date patches) openSUSE 12.2 Do not appear in: Ubuntu 10.04 Fedora 16 Fedora 17 Mike, Are up-to-date patches missing from your Debian 6 VM? I'm curious as to why we see different results. I suspect a change or fix in the cairo drawing library is at the root of these differences $ dpkg -l | egrep "libcairo|libgtkmm" ii libcairo2 1.8.10-6 The Cairo 2D vector graphics library ii libcairo2-dev 1.8.10-6 Development files for the Cairo 2D graphics library ii libcairomm-1.0-1 1.8.4-3 C++ wrappers for Cairo (shared libraries) ii libcairomm-1.0-dev 1.8.4-3 C++ wrappers for Cairo (development files) ii libgtkmm-2.4-1c2a 1:2.20.3-1 C++ wrappers for GTK+ (shared libraries) ii libgtkmm-2.4-dev 1:2.20.3-1 C++ wrappers for GTK+ (development files) > 12) Using the existing "Partition > Format to" to reformat an > existing partition produces the following error dialog. Is this > intended or a bug? > (-) Cannot format this file system to FSTYPE > A FSTYPE file system requires a partition of at least 0.00 B. I can confirm this behaviour when trying to "Partition > Format to" a 20 GiB ntfs partition to ext4. With this partition properties enhancement, we will be able to clean up the Partition menu by removing the following: Partition --> Format Partition --> Label Partition --> New UUID When the "Partition -> Format to" menu is removed then this should no longer be a problem. However it is surprising that this behaviour has changed. Thanks, Curtis
(In reply to comment #68) > > 4) What about 60? Its 12*5 and I get a close value to the golden > > ratio being 1,62 as the ratio of window height and width. But I > > think 24 would be OK, too. > > I have attached a screenshot with both 60 pixel and 24 pixel > spacing. My preference is for something less than 60 pixel > spacing. > > Mike, > > Do you have a preference? Go with the size that matches the gap in the middle of New Partition dialog. Don't worry about getting the golden ratio spot on as the dialog resizes when error messages and LVM details are added. > 7) Colored squares beside file system testing results: > > Do appear in: > > Kubuntu 12.04 > Debian 6 (this VM includes up-to-date patches) > openSUSE 12.2 > > Do not appear in: > > Ubuntu 10.04 > Fedora 16 > Fedora 17 > > Mike, > > Are up-to-date patches missing from your Debian 6 VM? > I'm curious as to why we see different results. > > I suspect a change or fix in the cairo drawing library is at > the root of these differences > > $ dpkg -l | egrep "libcairo|libgtkmm" > ii libcairo2 1.8.10-6 > The Cairo 2D vector graphics library > ii libcairo2-dev 1.8.10-6 > Development files for the Cairo 2D graphics library > ii libcairomm-1.0-1 1.8.4-3 > C++ wrappers for Cairo (shared libraries) > ii libcairomm-1.0-dev 1.8.4-3 > C++ wrappers for Cairo (development files) > ii libgtkmm-2.4-1c2a 1:2.20.3-1 > C++ wrappers for GTK+ (shared libraries) > ii libgtkmm-2.4-dev 1:2.20.3-1 > C++ wrappers for GTK+ (development files) I have the same version of those packages: $ dpkg -l | egrep 'libcairo|libgtkmm' | cut -c-60 ii libcairo-perl 1.070-1 ii libcairo2 1.8.10-6 ii libcairo2-dbg 1.8.10-6 ii libcairo2-dev 1.8.10-6 ii libcairomm-1.0-1 1.8.4-3 ii libcairomm-1.0-dev 1.8.4-3 ii libgtkmm-2.4-1c2a 1:2.20.3-1 ii libgtkmm-2.4-dbg 1:2.20.3-1 ii libgtkmm-2.4-dev 1:2.20.3-1 13) Too wide pitch for the file system usage figures. Compare the pitch between text in the top right and bottom right. The pitch in the top right feels too big. It looks like the pitch is being dictated by the higher elements on the left. The New Partition dialog manages to layout elements with different pitches and looks natural. Thanks, Mike
14) The partition graphic is too narrow. There feels like there is too much empty space on the left and right of the partition graphic. Can you try making it wider. Again using the New Partition dialog as a reference.
Created attachment 244363 [details] [review] two additional fixes (points 3 and 9) Hi Mike and Curtis, I assume these issues as "fixed": 1 2 5 6 8 10 11 3) Thank you Curtis for raising this point. I assume this is a general problem for setting a label when its possible and then reformatting to "formats" not supporting relabeling like lvm2 pv or cleared. I've added a commit to fix the issue. 4) Mike you're right, the size can vary, so 60 is not to be favored. I've measured 36 pixels for the new partition dialog (not in the code but in the UI rendering). About 4 pixels are added (by margins and so on), so 32 can get being written into the code. Its smaller than 60. Do you both agree to this value? 7) I've taken the "Partition > Format to" menu code for the drop down box. To try another code, I could chose a similar approach to the disk select dropdown box in the upper right corner of the main window. 9) Mike, I get your point now. I'm sorry as I made an unright statement in my last comment. The init_drawingarea method is not solely responsible for drawing. I've added a new method that was responsible for the regression. However, with my additional commit it should work. 12) The error message is not intended, but I don't know whether its a bug. It depends on whether we want to remove the obsolete features (like the "Partition > Format to" menu) or not. If we decide not to remove then its a bug. I have reviewed the issue and I think that when removing these obsolete features the root cause of the issue is removed too, so there is nothing to fix if the obsolete gui features are removed anyway. My assumption: In line Win_GParted.cc:450 an element of the "Partition > Format to" menu gets its event assigned when clicked on. Its assigned to the activate_format method. The problem is, that, as I have added the newestpartition argument to that method, the address of selected_partition is saved into that function pointer at ui init time, while the selected_partition variable gets assigned later on user interaction, so the address the function pointer holds is invalid, and the activate_format method gets a pointer somewhere into garbage land, so that invocation of newestpartition .get_byte_length() returns -1. A dirty fix (that supports the assumption) is to add newestpartition = selected_partition ; into the first line of the activate_format function. By that the newestpartition argument gets senseless of course. Greetings, Sinlu
We have issue 13 two times: 13a) (from comment 67) 13b) (from comment 69) Hi Mike, I have a question on 13b): Could you please explain what you mean with "elements on the right"? The bracket values (like "(12%)") or the Number values more on the left (like "4736.4 MiB")? Is the pitch between those two the one that is too large for you?
Hi Sinlu, 13b) Please see red arrows on the attached picture to see what I mean about the too wide text pitch for the File System Used, Unused and Unallocated in the top right corner. 14) Please see green arrows on the attached picture for approximate suggestion for the width of the partition graphic. -- I have also tested the original Partition Information dialog and it uses a fixed number of pixels to set the width of the partition graphic. The dialog grows a little wider based of the text displayed within it but the graphic remains the same width. Thanks, Mike
Created attachment 244377 [details] Partition properties picture showing (13b) text pitch and (14) narrow graphic Here's the picture to go with comment #73.
Created attachment 244380 [details] Screenshot panorama showing (9) partition colour painting issue 9) The drawing of the partition colour in the graphic is still broken. It is not initially drawn, but only later drawn by repainting for expose events. In the attached panorama of screen shots see how it is only painted after I have dragged the small terminal window over it.
Created attachment 244402 [details] [review] experimental fix to resolve issue 9 Hi Mike, 9) I can't reproduce the issue, but have tried to fix it. (in reply to comment 73) OK I get your points now.
Created attachment 244882 [details] [review] implementation of vertical independently shown entries Hi Mike, does the patch from comment 76 fix the issue? I've no option to tell as I can't reproduce it, but I have guessed what the issue can be caused by. 14) The Partition should be now displayed wider 13b) It is possible to decrease the vertical pitch, but then the problem appears that the table gets inconsistent in horizontal alignment. Vertical eye scan lines get disrupted. You can see my proposed implementation in the attached patch. I currently see no possibility of how to achieve both horizontal pitch alignment and vertical pitch independence (from the left), but I'm open on ideas. If I had to decide between the two options, I would personally prefer the state it was before the patch from this commit (except issue 14 resolval of course).
Created attachment 244883 [details] Disrupted eye scan lines The eye scan lines that are disrupted are shown red.
Hi Sinlu, 9) Drawing issues with partition graphic: Your fix from comment #76 works. (Reproduced and tested on Fedora 14 and Debian 6). 14) Partition graphic width: Your fix from comment #77 looks better. Just need to centre the text as is done with the partition graphic in the main window. 13b) Text vertical patch question: The patch for this doesn't compile on Debian 6. With my limited knowledge of Gtk layout, that it is based based on containing tables/boxes, I don't know how to lay it out without having the different vertical alignment per section you highlighted in the picture attached to comment #78. Lets stick with what we had before. Thanks, Mike
Created attachment 245101 [details] [review] move text into center, ensure compilability on CentOS Thank you Mike for testing my patchset yet again :-) 13a) I've removed the usage of the methods in question. Mike could you please try to compile on CentOS? 14) Text should be centered now.
Hi Sinlu, It appears that by committing the patch for bug #699452, I have created a conflict with your patch set in comment #80. $ git am bug691681-partition-properties.patch <snip> Applying: Improve Partition Information Dialog to be Partition Properties Dialog (Bug #691681) error: patch failed: src/Win_GParted.cc:716 error: src/Win_GParted.cc: patch does not apply Patch failed at 0002 Improve Partition Information Dialog to be Partition Properties Dialog (Bug #691681) <snip> Would you be able to rebase your patch set to work on the latest git repository master? Thanks, Curtis
Created attachment 245280 [details] [review] move text into center, ensure compilability on CentOS, rebased, increased column space rebased the patch from comment 80 to most recent git head.
Hi Curtis, thank you for pointing out the conflict. Thanks Sinlu
Hi Sinlu, I've quickly testing your patch set from comment #82. 13a) Doesn't compile on CentOS 5.9: Your patch works. Just need to squash the commit with the second patch so that every commit builds on CentOS 5.9 so the code is git bisectable. (Please ask if you don't know how to git rebase -i to squash a commit). 14) Partition graphic: Text is centred. Passed. 15) Applying operation doesn't open pending operations or enable Undo and Apply buttons: I suspect this is as a result of this fix applied to prevent GParted crashing: Only allow Undo and Apply after merging operations https://git.gnome.org/browse/gparted/commit/?id=4cc426c6cfb579548d432a90f12189494160a5f5 Thanks, Mike
Created attachment 245422 [details] [review] enure git bisectability on CentOS and fix rebasing I assume these issues as fixed: 1 2 3 (confirmation missing) 4 (confirmation missing) 5 6 8 9 10 11 13b 14 open: 7 12 13a 15 should be fixed now: 13a 15 Hi Mike, Thank you for your offer to help. You're right, 15) occured due to an error in the rebase done after your commit. I've amended the commit to resolve both 13a 15.
7) I've tried the approach with Gtk::Combobox I mentioned in comment 71, and I think if we use a Combobox for the filesystems we lose the ability to set the sensitivity of the rows. All of them would be selectable, even the filesystems a partition cannot be formatted to. I think no colored boxes with some configurations are better than this serious drawback.
Hi Sinlu, Coincidentally, I encountered a similar challenge while trying to get GParted to compile with gtkmm-3. Fortunately Kjell Ahlstedt answered my question to the gtkmm mailing list. See the following link: Re: ComboBox with some entries not selectable or grayed out https://mail.gnome.org/archives/gtkmm-list/2012-February/msg00041.html I'm not sure if this works with gtkmm-2 as I have not tried it. Curtis
Hi Sinlu, 7) When testing your patch from comment #85 on my kubuntu 12.04 distro, the "File system" ComboBox _does_ contain grayed out (not selectable) rows. For example I do not have the f2fs-tools package installed and the f2fs entry is greyed out. On which distro are you testing? Curtis
Hi Curtis, Thank you for this great link, it helps alot. My experimental changes to replace the Gtk::OptionMenu with Gtk::ComboBox are not included in the patch I posted in comment 85, but with the knowledge of how to make the elements (not) selectable I think I will post them soon. 7) I've applied the ideas from the link, and it compiles and works (at least for me). I have both gtkmm-3.0 and 2.4 installed. Does that positively affect the compilability? I couldn't find any include paths in make output that show usage of gtkmm-3.0. Sinlu
Created attachment 245595 [details] [review] Next try to colored boxes Now here's the promised patch. I've added one commit: Make colored boxes next to a filesystem in Partition Properties dialog appear on all OS (#691681)
Hi Sinlu, I've tested the following: 13a) Doesn't compile on CentOS 5.9 Passed. Every patch builds. 15) Applying operation doesn't open pending operations or enable Undo and Apply buttons: Passed. 9) Drawing issues with partition graphic: Unforutunately I've found another case where it doesn't work. Choose to reformat the file system to a different type. The FS specific border colour doesn't update. Dragging a window over the top causes the expose events to update it. Similar to before but with the extra step of choosing a different FS type. Confirmed on: Fedora 14, Debian 6 and CentOS 5.9 Set of strange behaviours so far only tested on Fedora 14. 16) Sometimes the choice to reformat as btrfs is grayed out on the drop down, other times it's available. 17) Switching between reformatting as a file system which can and can't be labelled causes the whole dialog to be resized. Width and height. I seem to be finding lots of display and other issues you don't. What distro and version are you developing on? Can you install a virtual machine with something older and do some testing yourself? All i'm doing is user testing you should be able to do. I haven't yet got to asking you to restructure the patch from one huge change followed by 15 and counting fixups into a reasonable set of incremental steps which can be code reviewed and makes sense in the git history. Mike
Created attachment 245677 [details] [review] Reorder and improve some commits, fix issue for unknown filesystems Hi Mike, Thank you for your patience with me, and thank you for your countless tests. I appreciate that you invest your time to test my patches to GParted. 9) Which patchset did you use for testing? The one from comment 85? I've encountered something similar after having changed to Combobox, so perhaps it is resolved already. Have you tried patch from comment 90? 16) To have a common base, please use the patchset from this commit for testing issue. When does (or can) the selectability of btrfs change, when it's constant? - opening the dropdown - selecting new fs - re-opening the properties window - restarting GParted 17) What filesystem do you have when you initially open the properties window? Does it have an UUID? Usually they are wider than the filesystem label textbox when it appears, so that the window only gets resized in height. What's your proposed behaviour? All-time display of the Textbox? Spacers when there isn't a Textbox? Smaller Textbox? (In reply to comment #91) > I seem to be finding lots of display and other issues you don't. There are some issues I know but I don't see as ones until you mention them, like 17). However, of course, not all issues you write are of this kind. I have the behaviour 17) on my distro, too. In the patch I post, I have added a fix for an issue I found while testing: Partitions with unknown filesystem get marked as "Btrfs". > What > distro and version are you developing on? I'm developing on Kubuntu 13.04. > Can you install a virtual > machine with something older and do some testing yourself? All i'm > doing is user testing you should be able to do. VMs need time to set up. So therefore I won't create such a big collection of VMs, but I'll install Fedora 14, and do some basic testing there, also to reproduce 16) However, I can't test every use case out there. Could you please still do some testing? > I haven't yet got to asking you to restructure the patch from one huge > change followed by 15 and counting fixups into a reasonable set of > incremental steps which can be code reviewed and makes sense in the git > history. I have split up the second commit, into one for Bug #690953 and two containig the changes of the original commit concerning this Bug. Then I moved the Partition name commit to be first, before the class rename commit. Additionally I improved some commit messages. I'll try to squash and improve some other commits, too. See comment 19 about why I didn't change past commits very often (until now). The move from one Information display to a Properties editing Window is rather large, and splitting it into multiple even-sized steps is hard when functionality should be preserved on all commits, but I'll try to give my best. Sinlu
Hi Sinlu, (In reply to comment #92) > Thank you for your patience with me, and thank you for your countless tests. > I appreciate that you invest your time to test my patches to GParted. Welcome. I understand it takes time to learn the unwritten rules we have about how we think best works. I'll reply to the issue numbers 9, 16, & 17 later, hopefully tomorrow. > I have split up the second commit, into one for Bug #690953 and two containig > the changes of the original commit concerning this Bug. Then I moved the > Partition name commit to be first, before the class rename commit. > Additionally I improved some commit messages. > I'll try to squash and improve some other commits, too. See comment 19 about > why I didn't change past commits very often (until now). > > The move from one Information display to a Properties editing Window is rather > large, and splitting it into multiple even-sized steps is hard when > functionality should be preserved on all commits, but I'll try to give my best. My main concern was that patch number 2 was too big a change for me to really understand. It has become apparent to me (and I think Curtis) that we need to have a pretty good understanding of the code we apply so at least we can have a go at debugging it. New code unfortunately means new bugs and users get upset when we loose their data when things go wrong. To me there was a big shopping list of changes in the second patch. I saw a number of obvious smaller incremental steps based on the commit message: Re-layout Partition Information into 2 column Partition Properties Add File System relabelling Add File System reformatting Add new UUID generation Add Partition naming support Making a single change in one commit makes it smaller and easier to understand. The sequence of commit messages become a story describing the change set. Additionally git bisect gets you to a smaller code change. Breaking code or functionality in the middle of a patch set is generally bad. Correcting brokenness when developing the patch set usually involves rebasing the correction into the relevant earlier commit. This makes changing existing functionality harder. On the other hand discovering limitations and enhancing the code later in the patch set adds to the understanding. (Rambled enough now). Whatever you think works. Mike
Hi Sinlu, We definitely appreciate your help to improve GParted. I agree with Mike's comment #93 and could not have stated it better. This has been a learning experience for us too. :-) Curtis
Created attachment 245686 [details] [review] Refurbish patchset commit messages and melt one commit with previous ones Hi Curtis and Mike, Thank you for your understanding. I've added a newest version of my patch where I mostly refurbished some commit messages and removed one commit with the rather less nice commit message "minor changes for Bug #691681" (and melted its changes with the others). I also removed one line of an unnecessary include. I admit these are only cosmetic changes and I want to do more improvement, by removing multiple changes of the same line. Though I probably won't reach the structure Mike expects when he reads the commit mesage of the "second" commit. I named the "second" (now its the 4th) commit like the bug summary because I think it contains the main changes of the patchset, where the functionality of the dialog actually changes. The order and structure of the patches after the 4th were motivated after the (rather long) history of this bug report, and your suggestions in it. Smaller commits which only change already changed lines are likely to be removed. Sinlu
Hi Sinlu, Testing of patch set from comment #95 ... 9) Drawing issues with partition graphic: 16) Sometime btrfs grayed out in reformat file system dropdown: Yes I was testing with patch set from #85. Can't re-produce either of these on Fedora 14 or Debian 6 any more. Closed. 18) Properties dialog uses 100% CPU in gpartedbin and Xorg: Just open the Partition Properties dialog. Don't do anything, just look at top. gpartedbin and Xorg using 100% CPU. Confirmed on Fedora 18, Fedora 14 and Debian 6. Git bisected to: fc91e87a8107a747afc41b31d7bd5e8b994fc7e5 Make colored boxes next to a filesystem in Partition ... Mike
Created attachment 245781 [details] [review] refurbish, refactor and fix an annoying CPU 100% for loop fixed: 1 ... 6 8 ... 11 13a 13b 14 15 16 open: 7 (colored boxes) 12 (remove redundant code and features) 17 (filesystem label resizes window) 18 should be fixed now: 18 7 Hi Mike, 7) Colored boxes appear for me in Fedora 14 now. In comment 67 (and my own tests too) they didn't. So there is a fix. 18) Thank you for bisecting. Could have noticed this by myself, too. In fact I heard my CPU fan was louder, but I didn't realize this had something to do with gparted or my patch. I think there is a while-true-loop: a) drawingarea_on_realize() gets called when the dialog opens b) drawingarea_on_realize() calls Draw_Partition() c) Draw_Partition() calls this ->queue_draw() d) this ->queue_draw() calls or raises drawingarea_on_expose() e) drawingarea_on_expose() calls Draw_Partition() The loop is from c) to e) The commit you bisected added c). I modify it a bit, moving the call to queue_draw() to a), and to filesystem_optionmenu_changed(). I've amended the commit: a3ab2df5854018770d4e7e80960048a89ecea559 Make colored boxes next to a filesystem in Partition ... For the patchset I've also further squashed commits, and added one refactoring commit. Hope it doesn't break anything :-), but my testing shows no breakage so far. The code at the commit: Make colored boxes next to a filesystem in Partition ... shouldn't have changed with my patch from comment 95. So if you checked out to this commit, and git diff SHAID to an older version of this commit (SHAID observable for example in testbuild.log), you should only see the changes I've described in this comment. If you and Curtis don't have new concerns I could start to remove redundant code now (resolves issue 12). Sinlu
Created attachment 245818 [details] Red Hat Anaconda installer, Edit Partition dialog 17) Switching between reformat as file system which can and can't be labelled resizes the whole dialog: Just happened to come across the answer. See the attached screen shot of the Red Hat Anaconda installer Edit Partition dialog. See the Mount Point entry box. It is always shown, but just enabled or disabled as required. This is how we should treat the File System label entry box.
19) Build broken on CentOS 5.9 by use of ComboBox::get_cells(): Suggest use of get_cells(), and therefore showing of file system coloured boxes, is conditional on it being available. Then old CentOS 5.9 will at least build. See HAVE_GTK_SHOW_URI and HAVE_GET_MESSAGE_AREA in configure.ac as examples of how to do a check. Looks like gtkmm >= 2.12 is needed, but CentOS 5.9 only has gtkmm 2.10. https://developer.gnome.org/gtkmm/stable/classGtk_1_1CellLayout.html#a667bb6d38468750a30600575c540b9d3 Build breaks on CentOS 5.9 with commit: 8dbd589d598045ebfff3f75e25d4416a189f06b1 Make colored boxes next to a filesystem in Partition Properties ... Build error: if g++ -DHAVE_CONFIG_H -I. -I. -I.. -pthread -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/gtkmm-2.4 -I/usr/lib/gtkmm-2.4/include -I/usr/include/glibmm-2.4 -I/usr/lib/glibmm-2.4/include -I/usr/include/gdkmm-2.4 -I/usr/lib/gdkmm-2.4/include -I/usr/include/pangomm-1.4 -I/usr/include/atkmm-1.6 -I/usr/include/gtk-2.0 -I/usr/include/sigc++-2.0 -I/usr/lib/sigc++-2.0/include -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/lib/gtk-2.0/include -I/usr/include/cairomm-1.0 -I/usr/include/pango-1.0 -I/usr/include/cairo -I/usr/include/freetype2 -I/usr/include/libpng12 -I/usr/include/atk-1.0 -DGPARTED_DATADIR=\""/usr/local/share"\" -DGNOME_ICONDIR=\""/usr/local/share/pixmaps"\" -DGNOMELOCALEDIR=\""/usr/local/share/locale"\" -Wall -g -O2 -MT Dialog_Partition_Properties.o -MD -MP -MF ".deps/Dialog_Partition_Properties.Tpo" -c -o Dialog_Partition_Properties.o Dialog_Partition_Properties.cc; \ then mv -f ".deps/Dialog_Partition_Properties.Tpo" ".deps/Dialog_Partition_Properties.Po"; else rm -f ".deps/Dialog_Partition_Properties.Tpo"; exit 1; fi Dialog_Partition_Properties.cc: In member function ‘void GParted::Dialog_Partition_Properties::init_filesystems_menues()’: Dialog_Partition_Properties.cc:276: error: ‘class Gtk::ComboBox’ has no member named ‘get_cells’ make[2]: *** [Dialog_Partition_Properties.o] Error 1 make[2]: Leaving directory `/home/centos/programming/c/gparted/src' make[1]: *** [all-recursive] Error 1 make[1]: Leaving directory `/home/centos/programming/c/gparted' make: *** [all] Error 2
Hi Mike, Thank you for testing. 19) Using the Preprocessor is a good idea. I'll use, as suggested, the unreliable first implementation of colored boxes as a fallback: Add FS color boxes to partition properties (#691681) Just a question: CentOS 5.9 has gtkmm 2.10. But why are there include paths like: /usr/lib/gtkmm-2.4/include This made me think that CentOS 5.9 has gtkmm 2.4 when reading comment 67, so I first wrongly debarred gtkmm version problems.
Hi Sinlu, > Just a question: CentOS 5.9 has gtkmm 2.10. But why are there include paths > like: > /usr/lib/gtkmm-2.4/include > This made me think that CentOS 5.9 has gtkmm 2.4 when reading comment 67, so I > first wrongly debarred gtkmm version problems. I too found this confusing at first. If I recall correctly the gtkmm API was "locked" at 2.4, even though the gtkmm versions continue to increase after that. That is the reason for the hard-coded paths for the gtkmm 2.4 API. To summarize, gtkmm-2.10 uses the gtkmm 2.4 API. Curtis
Created attachment 245832 [details] [review] Remove unneccesary line, enhance compilability on CentOS 5.9, always display fs label entry Hi Mike and Curtis, Thank you Curtis for answering my question. 12) I want to start discussion (again) about which redundant features of GParted to remove: - Partition -> Format to - Partition -> Label - Partition -> New UUID - File src/Dialog_Partition_Label.cc - File include/Dialog_Partition_Label.h See point 7 from comment 17 17) Implemented suggestion from comment 99 In Kubuntu 13.04 its hard to distinguish an empty greyed out Entry from a non greyed out one. Therefore I've added some text to the greyed out Entry. 19) I've amended the commit: Make colored boxes next to a filesystem in Partition Properties ... I also removed two unneccesary lines in the commit: Improve Partition Information Dialog to be Partition Properties ... Greetings Sinlu
Created attachment 245844 [details] Screenshot panorama showing (16) formatting as some file systems grayed out 18) Properties dialog uses 100% CPU in gpartedbin and Xorg: Confirmed fixed. 16) Sometimes the choice of some file systems is grayed out: See attached screen shots. First shot is reformatting sda7 from ext2 and any supported file system is available. Second shot is reformatting sda11 from ext2 and the possible choices is much less! Only seen on my Fedora 14 desktop so far. Git bisected it to this commit which is the first one adding the reformat drop down: 8093c05f254817a49ebb80da0a8e1a6016e85378 Split partition properties dialog into two columns (#691681)
19) Build broken on CentOS 5.9 by use of ComboBox::get_cells(): Confirmed fixed on CentOS 5.9. I was pleasantly surprised to see coloured boxes next to the file system names in the open dropdown of the file system format combobox. Just no coloured box when the combobox was closed. So it really was hit and miss whether the older method using Gtk::Menu without get_cells() worked, depending of the gtkmm version I presume. I just assumed it it wouldn't work on any older versions when I didn't seem them on Fedora 14 and Debian 6. 20) Build broken on CentOS 5.9 by use of get_sensitive(): At commit: df75f9c35199cf43b167b988309951630c82cfe1 Always display filesystem label entry in Partition ... get_sensitive() requires gtkmm >= 2.18, but CentOS 5.9 only has gtkmm 2.10. https://developer.gnome.org/gtkmm/unstable/classGtk_1_1Widget.html#a9a8ffa25c5857334efa3eb080d344319 Having a look at filesystm_optionmenu_changed() where the function is used it doesn't look like it would be very hard to decide what needs reading from or writing into the label text entry box without using get_sensitive(). if g++ -DHAVE_CONFIG_H -I. -I. -I.. -pthread -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/gtkmm-2.4 -I/usr/lib/gtkmm-2.4/include -I/usr/include/glibmm-2.4 -I/usr/lib/glibmm-2.4/include -I/usr/include/gdkmm-2.4 -I/usr/lib/gdkmm-2.4/include -I/usr/include/pangomm-1.4 -I/usr/include/atkmm-1.6 -I/usr/include/gtk-2.0 -I/usr/include/sigc++-2.0 -I/usr/lib/sigc++-2.0/include -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/lib/gtk-2.0/include -I/usr/include/cairomm-1.0 -I/usr/include/pango-1.0 -I/usr/include/cairo -I/usr/include/freetype2 -I/usr/include/libpng12 -I/usr/include/atk-1.0 -DGPARTED_DATADIR=\""/usr/local/share"\" -DGNOME_ICONDIR=\""/usr/local/share/pixmaps"\" -DGNOMELOCALEDIR=\""/usr/local/share/locale"\" -Wall -g -O2 -MT Dialog_Partition_Properties.o -MD -MP -MF ".deps/Dialog_Partition_Properties.Tpo" -c -o Dialog_Partition_Properties.o Dialog_Partition_Properties.cc; \ then mv -f ".deps/Dialog_Partition_Properties.Tpo" ".deps/Dialog_Partition_Properties.Po"; else rm -f ".deps/Dialog_Partition_Properties.Tpo"; exit 1; fi Dialog_Partition_Properties.cc: In member function ‘void GParted::Dialog_Partition_Properties::filesystem_optionmenu_changed()’: Dialog_Partition_Properties.cc:363: error: ‘class Gtk::Entry’ has no member named ‘get_sensitive’ Dialog_Partition_Properties.cc:370: error: ‘class Gtk::Entry’ has no member named ‘get_sensitive’ Dialog_Partition_Properties.cc:376: error: ‘class Gtk::Entry’ has no member named ‘get_sensitive’ make[2]: *** [Dialog_Partition_Properties.o] Error 1 make[2]: Leaving directory `/home/centos/programming/c/gparted-test/src' make[1]: *** [all-recursive] Error 1 make[1]: Leaving directory `/home/centos/programming/c/gparted-test' make: *** [all] Error 2
16) Sometimes the choice of some file systems is grayed out: Worked out what is happening. The reformat as file system combobox is also making items insensitive based partition size being smaller than the minimum file system size, but it doesn't do this if the partition size is too big for the file system. In the too large case what I think is an existing error dialog appears after apply. The old Format as menu makes all supported file systems sensitive and shows an error dialog if the partition is too small or too large for the file system type. I think that this is a trap users will easily fall into and not understand why the file system choice is disabled when it is supported, but just the wrong sized partition. I think we need an error dialog if the partition is either too small or too large to inform the user why they can't pick it as the old Format as menu does.
21) Crash when opening properties of btrfs: Just open properties dialog of a btrfs file system and GParted crashes. Tested on: Fedora 14, Debian 6. 22) Glib and Gtk critical assertion errors when opening properties of unknown partition: Just open properties dialog of an unknown partition. Get these assertions: (gpartedbin:3520): Gtk-CRITICAL **: gtk_list_store_get_value: assertion `VALID_ITER (iter, list_store)' failed (gpartedbin:3520): GLib-GObject-CRITICAL **: g_value_get_boolean: assertion `G_VALUE_HOLDS_BOOLEAN (value)' failed (gpartedbin:3520): GLib-GObject-CRITICAL **: g_value_unset: assertion `G_IS_VALUE (value)' failed (gpartedbin:3520): Gtk-CRITICAL **: gtk_list_store_iter_next: assertion `GTK_LIST_STORE (tree_model)->stamp == iter->stamp' failed It could well be that (21) and (22) are the same thing as for reiser4 on Debian 6 I get a crash and on Fedora 14 I get the assertions. That's probably enough testing for one day. Mike
Created attachment 246036 [details] [review] fixed various issues Hi Mike, Thank you for testing. 16) You're right, its best to inform the user about size problems. I've removed the minimum requirement in the commit: Make colored boxes next to a filesystem in Partition Properties dialog appear on all OS (#691681) It still exists in history but that shouldn't be an issue, should it? 20) I have amended the commit, and replaced get_sensitive() with property_sensitive(), which should be available on CentOS 5.9 22) The errors shouldn't appear on console now, I've used the page https://developer.gnome.org/gtkmm-tutorial/unstable/sec-iterating-over-model-rows.html to get an error-less for-loop in init_filesystems_menues() I've moved the invocation of init_filesystems_menues() a bit down. When it was called at the old position, it might have called filesystem_optionmenu_changed, which then dereferences the non-initialized pointer check_new_fs to access the method get_active(), if the first element of the || returned false. I got (unreproducable) errors like this on opening of the dialog: (gpartedbin:15076): Gtk-CRITICAL **: IA__gtk_toggle_button_get_active: assertion `GTK_IS_TOGGLE_BUTTON (toggle_button)' failed Also I made the method get_fslabel() more intelligent, so that it never returns "not editable for selected file system".
Hi Sinlu, Recently I committed some work by Mike to rename all of the include headers to follow the same naming standard. See bug #539297 comment #5. Would you be able to update your patch set (where you rename the Dialog_Partition_Info.h/cc) to match this new include naming standard? For example: #ifndef GPARTED_DIALOG_PARTITION_PROPERTIES_H #define GPARTED_DIALOG_PARTITION_PROPERTIES_H ... #endif /* GPARTED_DIALOG_PARTITION_PROPERTIES_H */ Thanks, Curtis
Created attachment 246113 [details] [review] rebased, removed small comment mistake Hi Curtis, thank you for pointing this out. I've rebased the patchset and removed a small copy-paste mistake in a comment in my change to configure.ac. Sinlu
Hi Mike, do you want the removal of the features in a single big commit or in one commit each? - Partition -> Format to - Partition -> Label - Partition -> New UUID
Separate commits please. (They can be easily squashed together in git, but it's much more work to separate them if required).
20) Build broken on CentOS 5.9 by use of get_sensitive(): At commit: 21) Crash when opening properties of btrfs: 22) Glib and Gtk critical assertion errors when opening properties of unknown partition: Fixed. Hi Curtis and Sinlu, Can I ask you to put your tester / user hat on please. Create half a dozen partitions and then go through them all changing the UUIDs, then changing the label and finally reformatting them. Use the new Partition Properties, then use the old code. I want you to really experience the new Partition Properties and compare it to the existing methods of making changes. Then we can make a more informed decision about which features to remove. Thanks, Mike
23) Allows extended partition to be reformatted with a file system
Created attachment 246189 [details] [review] disallow reformat for extended, make traditional reformat bug free again Hi Mike, I'll do some testing. I've resolved the issue with formatting using the traditional method (comment 67), and 23), too.
Created attachment 246454 [details] [review] Use create_with_label flag from FS struct I've replaced the usage of read_label with create_with_label. See Bug 701569.
Hi Sinlu, In comment #112, you asked Mike and me to try testing again to make a more informed decision on which way to proceed. Have you found that the partition properties dialog method does not work as well as the original code? Or is there something else you have discovered? Unfortunately I have not had time to do extensive testing on this patch and might not have sufficient free time for another month. If possible I would prefer if you test this patch extensively first before I begin testing again. Regards, Curtis
Hi Curtis, It was actually me that asked you and Sinlu to compare the usability of the new and old code in comment #112. I find that for the simple operations of changing the UUID or formatting to a file system type, it takes several times more UI steps using Partition Properties compared to the old methods. Therefore it seems unfriendly from a usability point of view. (Changing a label is equally quick in the new and old methods). I would like you and Sinlu to use both methods and compare them now that the code works well enough to try it out. Then we can have an informed discussion about what old menu items to remove and even what new methods to keep in the Partition Properties before fixing the remaining bugs. Thanks, Mike
Hi Mike, Oops, my bad. Thank you for the clarification Mike. When I have a chance to look closely at this I will, though this might not be for another few weeks as I am busy with some stuff that I can only do in summer. Hopefully you get a chance to enjoy some warmer weather too. Cheers, Curtis
Hi Curtis and Mike, I've done some comparison of the two methods as Mike suggested, although no bug-fetching (yet). If you want to do only one operation with a partition, then the old method is faster. There is not an additional window where you have to click through and search the "Apply"-Button. But if you want to do multiple changes at once, then the new method is faster, and you have always a good reply about what your changes affect. For example, you see before you've added a reformat change, that the label gets shortened when you select a new filesystem. Also note, that reformatting is easier with the old method than the new method, but this also eases to accidentally reformat a partition, deleting all files. Accidental Label changes can be easily reverted, and UUID changes cause not *this* big of harm. With the exception of Filesystems, the old method hides away the actual values behind their meaning. Users who search for the values, and are less interested in what they are named in gparted (For example an user wanting to change the only partition's label of an USB-HDD might say: "I want to change the disks name from 'Alice's disk' to 'Bob's images'", without knowing that the "name" has to be changed via the "Partition->label" command) might feel an advantage of the new version, as he has only to remember the single point "Properties". Smaller flaw I've found: 24) When selecting a filesystem which supports a long label, then selecting a shorter label FS, then selecting a longer label FS again cuts off the label typed in at the beginning. A solution would be perhaps to store the label with its original length, getting overwritten when a label is changed. Greetings, Sinlu
Hi Mike and Sinlu, Thank you for your perseverance and questions with this report. Today I finally tested changing labels, UUIDs, and formatting file systems with the new partition properties, and with the previous method. Thank you Mike for asking Sinlu and me to have another look at this. I have come to discover that some things are better with the new method, and some are better with the old. Things I like with the new partition properties: - improved layout of the information versus the old partition dialog - ease of changing partition labels and volume labels With label changing, the user does not even need to know the type of the label (e.g., partition or volume). Thank you Sinlu for pointing this out in comment #119. Things I like with the old menu method: - reformatting a file system (this is also faster but misses setting a label) - setting a new UUID (this is faster) Further reasoning: With reformatting a file system, I think my initial idea of placing this in a properties dialog was wrong (my bad). The reason is that a file system is not a simple property that can be changed without other consequences. In fact the major consequence is that all data is lost! As such I do not think reformatting file systems should be possible in a _properties_ dialog. Further, I think it is too easy to reformat a file system with the old menu method. There should be some warning indicating that all data in the previous file system will be lost. Such an enhancement should be handled in a new bug report though, and not this one. With setting a new UUID, I could go either way (new partition properties or old menu system). Possible changes: a) Remove ability to reformat file system from partition properties b) Remove ability to change UUIDs from partition properties Do you agree with the above possible changes? I look forward to your comments Sincerely, Curtis
Hi Curtis, (In reply to comment #120) > Further, I think it is too easy to reformat a file system with the > old menu method. There should be some warning indicating that all > data in the previous file system will be lost. Such an enhancement > should be handled in a new bug report though, and not this one. I'm not sure this is warranted on top of the existing dialog checking for confirmation when applying pending operations. As a reminder it looks like this: Apply operations to device -------------------------- . Are you sure you want to apply the pending operations? /!\ --- Editing partitions has the potential to cause LOSS of DATA. You are advised to backup your data before proceeding. [ Cancel ] [ Apply ] Given an operation of format partition to new file system. What else can the user expect except data loss? (Rhetorical question :-). Mike
Hi Mike, Thanks for the reminder. It definitely helps to have another perspective. Sometimes I fall into the trap of too many warnings, which can lead users to ignore all warnings. We could leave the reformat operation as it is, or as an improvement we could add a dialog that would permit the user to specify a label at time of format. Many file systems let you set a label after formatting, but with some FS's we do not have a method to set just the label (e.g., reiser4, hfs/+). Curtis
Created attachment 252326 [details] [review] Store label to not permanently trim it at selection of short-label fs, remember reformat descision Hi Curtis and Mike, I believe at least the UUID option should remain, as a properties dialog should always be capable to also edit the properties. Without even the UUID checkbox the dialog becomes to a reordered information dialog which can only set labels for partition and filesystem. The old methd is faster for UUIDs. What speaks against redundancies? There are currently three methods (one obvious, one fast and one in between) to invoke the partition properties dialog for a given partition. I also can live with the reset UUID in both dialog and partition->properties menu. My commit fixes 24), and it also makes the partition properties dialog recognize at invocation time whether the selected partition was marked for reformat. This makes it possible to set the label of a fs, when it supports new labels only at format time, and the fs was previously marked to be reformatted. Note the feature when the reformat checkbox gets unchecked after the dialog has been opened after specifying reformat or creation. When we really decide to remove the format drop-down, we loose this feature. Curtis the dialog you suggested in comment 122 is a good idea. An improvement would be, to selectively show the Dialog only for reformat-time-only-label filesystems, explaining that this filesystem only supports setting the filesystem at reformat, but this is not the last chance as the label can still be edited in the properties dialog. This raises awareness for the users that the filesystem label can only be edited at reformat. Showing such a message when the user selects from a drop down list, where the effects are visible at once, violates of course the least suprise principle. A simple dialog informing the user that the label can only be set before the apply operations button is pressed without edit possibility would be an idea, too. Either way, such a dialog is probably something for a new bug. If we decide for a format drop-down or at least for a reformat checkbox with a label instead of drop-down, I will fix 25: 25) disabling the reformat checkbox after having enabled it and re-opened the properties dialog makes the wrong filesystem appear in the greyed out drop-down.
Created attachment 252327 [details] [review] Store label to not permanently trim it at selection of short-label fs, remember reformat descision Oh, sorry, wrong patchset
Hi Sinlu and Mike, Responses follow in-line. (In reply to comment #123) > I believe at least the UUID option should remain, as a properties > dialog should always be capable to also edit the properties. Without > even the UUID checkbox the dialog becomes to a reordered information > dialog which can only set labels for partition and filesystem. If we are planning in the future to be able to edit or set a specific UUID, then I agree that the ability to change the UUID should remain in the partition properties dialog. This would be a good enhancement so I would be okay with leaving the change UUID checkbox in the partition properties dialog. > The old method is faster for UUIDs. What speaks against > redundancies? My apology for not being clear. I think we should have only one method to change items such as the UUID. By having only one method we will simplify the user interface. For example we should either use a menu option, _OR_ the partition properties dialog to change UUID. I will clarify this with a list at the end of this post. > There are currently three methods (one obvious, one fast and one in > between) to invoke the partition properties dialog for a given > partition. What are the three methods? I can think of only two. - Change UUID using "Partition -> New UUID" - Change UUID in partition properties using checkbox (Note: Partition properties can be opened in three ways if this is what you meant. For example: - from menu "Partition -> Properties" - from right click menu "Properties" - by double clicking partition ) > I also can live with the reset UUID in both dialog and > partition->properties menu. For simplicity sake, I think it is best to have only one method to change a value. PROPOSED LIST OF METHODS TO EDIT/CHANGE ITEMS: a) Reformat file system to be available through "Partition -> Format to" only. Note: a new bug report to be opened to create reformat dialog to permit setting label at format time. b) Partition (GPT) label to be editable through partition properties dialog only. c) File system (volume) label to be editable through partition properties dialog only. d) New UUID to be available through partition properties "change UUID" checkbox only. Before making any changes to the code for the above list, we should all agree on the way to proceed. I look forward to your comments Mike and Sinlu. Curtis
Hi Curtis, Thank you for your fast reply. (In reply to comment #125) > > (In reply to comment #123) > > There are currently three methods (one obvious, one fast and one in > > between) to invoke the partition properties dialog for a given > > partition. > > What are the three methods? I can think of only two. > > - Change UUID using "Partition -> New UUID" > - Change UUID in partition properties using checkbox > (Note: Partition properties can be opened in three ways if > this is what you meant. For example: > - from menu "Partition -> Properties" > - from right click menu "Properties" > - by double clicking partition > ) > Sorry when I have expressed myself wrongly. With three methods I meant what you already suggested: fast: double clicking a partition obvious: menu "Partition -> Properties", as you only need to read the labels in between: right-click menu Your list looks good to me. a) I can replace the drop-down with a simple colored box next to the filesystem name, like we already have it in the filesystem column in the main window. b) fixes Bug #690953 c) making the partition and filesystem label editable at the place makes it easier for the user too see a difference between them. d) ensures extendability I'd only modify the Note at a): Note: a new bug report to be opened to create dialog to make the user aware that labels can be set only at format time for the filesystem he has chosen. The label can still be modified in the properties dialog before the format operation is executed, and the label operation then gets merged into the format operation. With the patchset from comment 124 this also works for label-only-at-reformat filesystems, and functionality is constructed to remain intact even when I remove the checkbox and the drop-down. Therefore I think that an additional dialog where the user can specify the label is redundant ( and it contradicts c) ), a simple hinting dialog would be enough. I think this should be clarified before we open the bug. Greetings Sinlu
Hi Sinlu, It appears that we agree on how partition properties should work, which is great. I look forward to Mike's input on this too when he has a chance. > I'd only modify the Note at a): > > Note: a new bug report to be opened to create dialog to > make the user aware that labels can be set only at format time > for the filesystem he has chosen. > > The label can still be modified in the properties dialog before the format > operation is executed, and the label operation then gets merged into the > format operation. With the patchset from comment 124 this also works for > label-only-at-reformat filesystems, and functionality is constructed to remain > intact even when I remove the checkbox and the drop-down. > Therefore I think that an additional dialog where the user can specify the > label is redundant ( and it contradicts c) ), a simple hinting dialog would be > enough. > > I think this should be clarified before we open the bug. Agreed. I currently have a differing opinion. If we are creating a dialog for reformatting a partition then I think it would be more user friendly if we let the user specify a new partition label in the dialog instead of indicating that partition properties must be used to change the label before the user clicks apply. I see reformatting a partition as being similar to creating a partition with a file system. In the create partition dialog we permit setting a label, and I think this would be good for the reformat dialog to maintain consistency. In arriving at this opinion I tried to think of a user going through the steps to reformat a partition. I think there would be less onus placed on the user if we let them set the label in the reformat dialog. Curtis
Hi Curtis, You're right that a dialog where you can directly specify the label is more user comfort than a hinting dialog. Therefore you're right, a dialog you can specify the label is the better option. I wanted the dialog only to appear for filesystems that allow labeling only at reformat. Instead I now think that the dialog should always appear, as it otherwise it violates the least suprise principle. Also, most users want to set the label at reformat. Sinlu
REPOST of Same Information as in bug #690953 comment #9 Hi Sinlu and Mike, For the past while this enhancement has been stalled out, and I have been wondering to myself why this is. Being the project leader I blame myself for this, and hence have tried to figure out how this came to be. Following are my thoughts on what went wrong, and suggestions to get back on track. WHAT WENT WRONG (THE STORY) =========================== The addition of partition name support originally started when Sinlu proposed the following enhancement. Bug #690953 - Partition name support My concern at the time was that users could get confused between volume labels (which GParted supports), and partition labels (which are the subject of the enhancement). The confusion would arise because GParted incorrectly refers to volume (or file system) labels as partition labels. Two suggestions for addressing this were suggested, and after discussion it was decided that a Partition Properties dialog would be the preferred method of implementation. WRONG TURN 1 ------------ With the realization of the clash between label names, we should have addressed the layout of the Partition Information dialog at this time to address the confusion. In the report I noted that there was an existing problem that the Re-Format operation did not support labelling and suggested that we could fix this at the same time. WRONG TURN 2 ------------ The addition of a Re-Format operation to a Partition Properties dialog was not well thought out. In hindsight I think we should have left this problem until later; however, we might not have learnt this without the work-in-progress patch prototype. From here a new bug report was created to convert the Partition Information dialog into a Partition Properties dialog. Bug #691681 - Improve Partition Info dialog to be Partition properties dialog Many patches were written to implement the Partition Properties. Some of the problems with the patch set, such as loss of history because at one time the information dialog was not renamed, were later addressed. WRONG TURN 3 ------------ We tried to include too much all at once. For example in addition to converting from an Information dialog to a Properties dialog, we added the partition name change, and re-format operation. We continued work on this change with some less than clear communication. WRONG TURN 4 ------------ Clear communication should have been made by me on how the Properties dialog should work. Partly as a consequence the patches submitted were not as well tested as they should have been. This also resulted in more testing by reviewers thus consuming more time which could have been more productively spent. Again we continued working on this change. WRONG TURN 5 ------------ We continued changing the patch set before we had landed the GUI layout. Ideally we should have agreed on the GUI layout early on. Having said that I think that our prototyping helped flush out the final layout. We agreed that 800x600 would be our minimum target screen resolution. From here we continued building on the patch set. WRONG TURN 6 ------------ The patch set is now large, adds and removes functionality such as re-format, and is difficult to review. In conclusion some good has come out of this process. For instance developing this patch set has helped to arrive at some standards and layouts. On the downside though, the resulting patch set does not represent a clean implementation of the changes needed to take the dialog from Partition Information to Partition Properties. SUGGESTION TO GET BACK ON TRACK =============================== I think to get this enhancement back on track we should consider the existing patch set as a prototype only. By this I mean that we should create a new patch set that implements changes in the following manner: 1) Create a patch to re-organize the existing Partition Information dialog so that it matches with our agreed GUI layout. 2) Review, rework if necessary, and approve this patch/set. 3) Create a patch to convert the Partition Information dialog into a Partition Properties dialog. Note that only existing functionality should be converted, such as volume labelling and setting a new UUID. The "Partition --> Label" and "Partition --> New UUID" menu option retirement should be one part of this patch set. 4) Review, rework if necessary, and approve this patch/set. At this point this report can be closed. 5) Use "bug #690953 - Partition name support" to create a new patch that adds Partition Name support. 6) Review, rework if necessary, and approve this patch/set. At this point bug #690953 can be closed. Mike and Sinlu, Does this sound like a reasonable approach to get this enhancement going again? Please feel free to comment on what might be right or wrong with my suggestions. Curtis
Hi Curtis, Of course I'm to blame that I didn't do enough testing before submitting patchsets. The larger a patchset, the more time has to be invested in coding instead of testing. There is a point where starting over is more efficient than fixing. I agree with you that this point is reached. The patchset doesn't look nice. I agree with your 6 steps. However I have a question: Where to include all the additional improvements, like label on reformat? I'd propose to only add those which are harder to avoid than to implement. I doubt I want to participate as code writer in all of the new work (however feel free to use my already published code). If you don't mind I'd volunteer for 5). Mike, do you want to join discussion about the next steps? I'd like to hear your opinion, too. Greetings Sinlu
Hi Sinlu and Curtis, I think GUIs are hard. IT is easy to use a GUI and find issues. It is hard to code them right. The design we came up with had lots of widgets and operational elements. Making all that change in one go became too complicated. It's always best to work on smaller bits, get them tested and upstream. It provides positive feedback of progress. I think Curtis' steps are the best way forward. (In reply to comment #130) > However I have a question: Where to include all the additional improvements, > like label on reformat? > I'd propose to only add those which are harder to avoid than to implement. Leave them out. Have a design which doesn't preclude their implementation. Maybe you'll want to add them later, maybe you won't. Mike
Hi Sinlu and Mike, I would be okay with Sinlu doing step 5 from comment #129. I think we should get step 1 from comment #129 done first though to reduce the confusion between the two types of labels. From searching through the bug reports I discovered that we currently have two open reports for the Partition Information dialog: Bug #690542 - Partition Information Dialog Warning not readable Bug #705596 - Partition Information Dialog - let user copy warnings Since these two reports already exist, I propose that we do step 1 from comment #129 using the first report (bug #690542). In this way the improved layout of the partition information dialog can be committed and become useful right away. We will not need to wait for when/if we convert the partition information dialog to a partition properties dialog. If no-one is interested in step 1 from comment #129, then I can take a look at it. Otherwise I am okay to step back and let someone else tackle step 1. Curtis
Hi Sinlu and Mike, It's been quite a while, but I finally got around to creating a patch to implement a two-column partition information display dialog. See Bug #690542 - Partition Information Dialog Warning not readable After having some time to think more about this report, I came up with another alternative to implement partition label support (not to be confused volume label support already available in GParted.) More specifically, what do you think about enhancing the partition label dialog to be able to set both the volume label and/or the new partition label? This would make for a simpler editable dialog than the complexity of the original solution we attempted in this report. Curtis
Thank you Sinlu and Mike for all your work to migrate the Partition Information dialog to be a Partition Properties dialog. Your efforts enabled us to explore a potential solution, provided a valuable learning opportunity, and helped forge the path to an accepted solution. GPT partition name support was added with the following report: Bug 741424 - Add support for GPT partition names Since support for GPT partition names was the initial reason for opening this report, I will close this report as a duplicate of the solution that was committed to the git repository. *** This bug has been marked as a duplicate of bug 741424 ***