GNOME Bugzilla – Bug 690542
Partition Information Dialog Warning not readable
Last modified: 2014-06-10 17:17:38 UTC
Created attachment 231968 [details] The screen-shot which shows the bug How it happened: I tried to install Ubuntu 12.10 on an external hard-disk, which did happen successfully. When I tried to view the filesystem in gparted, I saw one of the partitions had an error. The problem or the bub: When an error or warning is displayed in any of the device partitions, and you right-click on the partition and click on "Information", a dialog box pops up, that is not resizable. The description of the warning is described in a frame, which is not scrollable, so part of the description goes below the screen and you are not able to read it completely. The warning shows information such as Filesystem UUID, free space, block count, fragment size and so on. Probable solution: Reparent the frame into a scrollable window or a similar widget.
Thank you Jobin for reporting this problem. I could have sworn that we already had an existing bug report regarding this problem, but for the life of me I cannot seem to find it. If you have a chance to implement the solution you suggested, then a git patch would certainly be considered.
Change bug title from: Warning not readable To: Partition Information Dialog Warning not readable
Created attachment 273366 [details] [review] Partition information warning not viewable patch (v1) The attached patch restructures the fields in the partition information dialog into two columns to minimize the vertical space required. The dialog is made resizable, the warning messages scrollable, and an initial size has been set to ensure display on a minimum 800x600 resolution screen. The design for the two column display originated from much work by Sinlu Bes and Mike Fleetwood in the following bug report: Bug 691681 - Improve Partition Info dialog to be Partition properties dialog More specifically the design is roughly based on a template from bug #691681 comment #49 with some changes: a) The Total sectors field does not also include size (e.g., GiB). b) The Status field was added under the File System section. c) The LVM2 PV details were placed under the File System section. ---------------------------------------------------------------------- Information about /dev/sdd1 *File System* Type: [ntfs | unallocated | etc] Used: ##.## MiB (0%) Label: ______________________ Unused: ##.## GiB (100%) UUID: XXXX-XXXX-XXXX-XXXX {Unallocated} ##.## MiB (#%) Status: [Mounted | Active | etc] Size: ##.## GiB {Volume group:} xxxxxx {Members:} xxxxxxxxxx yyyyyyyyyy *Partition* Path: [/dev/sdX1 | unallocated] First sector: #### Flags: xxxx, xxxx Last sector: ######## Total sectors: ######## {*!Warning*} Optional warning text that would span across both columns.... Close ---------------------------------------------------------------------- Where: [ ] indicates possible text entries { } indicates optionally displayed information The patch set does not comply 100% with the current GNOME HIG standards, but I think it is a good compromise in order to achieve improved readability. One item that I wasn't sure about was the placement of the Size field. The size field is based on the number of sectors and hence would be the same for both the file system and for the partition. Currently it is placed underneath Used/Unused/Unallocated which makes for a list of numbers that should add up to the value at the bottom of the list. However, when details about the usage cannot be determined, then the used/unused/unallocated fields are not displayed. This happens, for example, when viewing unallocated space on a drive. In this situation the size field is displayed at the top of a one item list. Perhaps the size value should always be displayed at the top to meet with the principle of least surprise? Please review and let me know your thoughts. Curtis
Hi Curtis, Done some initial UI testing. 1) There seems a lot of empty space with setting the dialog height. 2) Using tabs to layout two lots of text "508.00 MiB" and "( 50% )" to not overlay in a single widget seems like bad GUI practice. 3) Don't really like the double box around the warning messages. 4) Adding 12 PVs into a single VG causes the partition information and any warnings to be below the bottom of the dialog. On a 800x600 screen there is no way to resize the dialog enough to see them. 5) UUIDs are a lot wider than other text so cause a log of empty space in the dialog. Suggestions to try: A) Put the scroll widget around everything in the dialog, except the close button. (Use automatic vertical scrolling as already used for the warning messages). [Should addresses: (3) & (4)] B) Put the percentage figures in a extra column in the table. [Addresses: (2)] Not sure how to address (1) and limit the dialog to fit on a 600 high screen. Setting no height request will address (1) but the dialog could be a lot taller with lots of PV members and/or warnings. If this happens the dialog will at least be resizeable from from the top downwards allowing the user to drag the dialog up and scroll to the bottom. Don't know if there is any reasonable layout to address (5). Probably just have to live with it. What do you think? Mike
Hi Mike, Thank you for the prompt review, comments and suggestions. Your two suggestions (A & B) sound like good improvements to me. I will see about implementing both of these. I agree about (1) being difficult to handle. Either the dialog size is fixed at a minimum size for 800x600 screens which potentially has a lot of white space for thinks like unallocated space, Or the dialog is not fixed at a minimum size and it might be displayed taller in height than a 800x600 screen can handle. Either decision is a trade-off. I am leaning towards trying your suggestion of not setting a minimum height and then permitting the user to shrink the dialog top downwards and then drag the dialog up in an effort to see all of the information. With (5) I also don't see an easy way to work around the large width of UUID's. I will post a new patch set once I have implemented the changes. Thanks, Curtis
Created attachment 274303 [details] [review] Partition information warning not viewable patch (v2) Hi Mike, The attached patch cherry-picks the first three commits from patch v1. A fourth patch adds an extra column per suggestion (B). A fifth patch makes the dialog resizable, adds a scrollbar to the info and message section, and sets an initial height per suggestion (A). I did not include the graphic in the scrollable region because I thought the dialog looked better if this was always displayed. I did try skipping the initial height per our discussion, but when testing with fluxbox on GParted Live 0.18.0-2 I was unable to use the upper portion of the dialog to shrink the height. I discovered the same problem when testing with GNOME on Fedora 20. To work around this problem I had to set a minimum height. In an effort to minimize the amount of extra whitespace in the dialog, I added some logic to set initial height to one of two values. While developing the patch, I tried to get rid of the inset border around the scrolled window. Unfortunately adding set_shadow_type(Gtk::SHADOW_NONE) to the Gtk::ScrolledWindow does not seem to work. If you have any thoughts here then I am open to suggestions. Otherwise we might have to live with the inset border for the scrolled portion of the window. The patchset has been tested with: Fluxbox on GParted Live 0.18.0-2 (Debian Live) GNOME on Fedora 20 KDE on Kubuntu 12.04 Unity on Ubuntu 13.04 Now that we have made the partition information dialog scrollable, I think we should keep the dialog for info display only and not use the dialog as a future properties window for changing partition and volume labels. Please review when you have time. Thanks, Curtis
Just to let you know I've started to look at this. Mike
Hi Curtis, you're right, for me the line info_scrolled. set_shadow_type(Gtk::SHADOW_NONE); also doesn't work. When I do info_scrolled. set_shadow_type(Gtk::SHADOW_OUT); I get two borders. This is interesting, as when I remove the Gtk::ScrolledWindow, and do this ->get_vbox() ->pack_start( info_msg_vbox, Gtk::PACK_SHRINK ) ; instead, the border is gone. This means that somehow the ScrolledWindow causes this. I think this behaviour is caused by this code: https://git.gnome.org/browse/gtk+/tree/gtk/gtkscrolledwindow.c#n2939 Explanation here: http://stackoverflow.com/questions/14372148/scrolled-window-border#comment20013447_14374835 So either we create our own ViewPort or take the autogenerated one and set its borders. Sinlu
Thank you Sinlu for testing the scrollable inset border issue and providing links indicating that others have encountered this too. You've confirmed that I am not losing my mind. ;-) My initial thought is to live with the border in an effort to keep the code complexity to a minimum. By keeping the code simpler, it should make it easier to maintain long term and also enable an easier migration to gtkmm 3.x at some point in the future. I'm curious what your thoughts and Mike thoughts are on this too. Curtis
Hi Curtis, Sorry, but so far I've only been through the first 4 of 5 patches and not yet looked at the 5th which introduces scrolling and resizability. One recommendation: C) Patch number 3, Organise into two sets of name, value columns: In the middle of ::Display_Info() when a blank line is added, you assume that the left columns will have at least as many rows as the right by using top and bottom to define the location where the blank line is added. You should do this (in pseudo code): + top = max(top, topright); + bottom = max(bottom, bottomright); //one blank line table ->attach( * Utils::mk_label( "" ), 0, 5, top++, bottom++, Gtk::FILL ) ; A few suggestions I saw for improvements when reviewing the contents of the dialog: D) Remove the vgname from the status of an LVM2 PV now that the volume group name field is displayed immediately below after the reorganisation. E) Put all the LVM2 member device names into a single string separated by new lines and display in a single widget so that they can all be selected together. F) Optionally remove "(Not a member of any volume group)" text from the LVM2 PV status now that a blank volume group name is immediately below. (Not sure about this one). Will try and look the 5th patch soon. Mike
Created attachment 274720 [details] [review] Remove border demo patch Hi all, First, thanks to Sinlu for explaining the issue. I don't think I would have been able find it myself or work out this solution otherwise. After several hours of reading gtkmm and GTK+ documentation and the source code I have finally worked out how to remove the border from the viewport. It only take these 2 lines: GtkWidget *scrollable_child = gtk_bin_get_child(GTK_BIN(info_scrolled.gobj())); gtk_viewport_set_shadow_type(GTK_VIEWPORT(scrollable_child), GTK_SHADOW_NONE); Attached is my testing patch with a big fat comment to explain to myself what is happening and why. Thanks, Mike
Hi all, Thank you Mike for figuring out the 2-line remove border patch. After I tried it I couldn't believe how much cleaner the partition information dialog looked. In my opinion this is definitely an improvement over the version that contained the border. Also I appreciate and agree with (C), (D) and (E) in comment #10. I'll plan to try out (F) to see how it looks. I anticipate that it will provide a cleaner look and still provide the necessary information (a blank list of volume group members). Since I have visitors this holiday weekend, it will likely be Tuesday before I get a chance to work on these improvements. Curtis
Hi Curtis and Mike, Thank you Mike for finding the solution to remove the border. The dialog looks much better now. Some suggestions on the patchset: G) When resizing the window, the table stays at the upper right corner, but the partition graphic always stays at the center. The table should also stay centered. H) We could solve the UUID problem like github with their git urls. They encapsulate the url into a text box. Selecting and copying is still possible, and perhaps we can manage that on resize of the window we also resize the text box. When this is done, we could decrease the height of the dialog, abiding the golden ratio and reducing white space between the "total sectors" line and the close button. I think this modification would be worth the "more complexity". You can see the text box in action at: https://github.com/GNOME/gparted Sinlu
Created attachment 275152 [details] [review] Partition information warning not viewable patch (v3) Hi Mike and Sinlu, Thank you for reviewing patch set v2 and sharing your ideas for enhancement. Attached is patch set v3 which includes several of your suggested changes. Please review when you have time. Patch set v3 builds on v2 and contains the following changes: P1 is cherry-picked from v2 P2 is cherry-picked from v2 P3 is cherry-picked from v2 and implements (C) row number enhancement P4 is cherry-picked from v2 P5 is cherry-picked from v2 P6 includes Mike's remove border demo patch from comment #11 P7 changes vertical alignment for Utils::mk_label() from CENTER to TOP P8 implements (E) by placing LMV2 member names into a string with new lines P9 implements (G) by centering the info data below the partition graphic P10 implements (D) by removing vgname from the status field That leaves the following two suggestions that are not implemented in patch set v3: (F) Remove "(Not a member of any volume group)" text from status field With (F) I am on the fence. By keeping the not-a-member text it makes it explicitly obvious to a user that the LVM2 PV cannot be in use, ever. If we eliminate this not-a-member text, then a person less familiar with LVM2 might not readily note that a <blank> Volume group field means the same thing. I do not have a strong preference on (F) either way. Mike, since you have a more vested interest in (F) I think the decision should be yours. (H) Encapsulate UUID into text box similar to github commit IDs. Sinlu, by github git urls, did you mean the commit ids? For example: short: 5f6656f267 long: 5f6656f267151b452cc95e036f8ad138a153d397 With (H) I think we were concerned with the length of the UUID horizontally stretching the partition information dialog. The suggestion of encapsulating the UUID into a short text box could alleviate this problem. While testing I have found that the UUID is not the only field that horizontally stretches the dialog. In a few situations the status field does this as well. For example with an extended partition one might see "Busy (At least one logical partition is mounted)" or "Not busy (There are no mounted logical partitions)". Since the partition information dialog is intended to show detailed information about a partition, I think that hiding a portion of the information makes it slightly less useful. For example, with encapsulation a user could click on the UUID and copy and paste the UUID; however the full UUID would not be available in a screen shot. I am leaning towards not encapsulating the UUID. We could re-visit this later if the aesthetics of the partition information dialog become an issue. Your thoughts are appreciated. Curtis
Created attachment 275203 [details] [review] Partition information warning not viewable patch (v4) Hi Curtis and Sinlu, Leave optional ideas (F) and (H) not implement. Reasoning: (F) Remove "(Not a member of any volume group)" from status field Keep it. Agree that it's a useful explanation and it also fits with a similar message for extended partitions "Not busy (There are no mounted logical partitions)" (H) Encapsulate UUID into text box similar to github commit IDs Lets keep the wide fields and show the UUID in full. GIT has standing practice of showing and accepting short commit IDs in lots of places. There is no such presence for UUIDs. We should show the full UUID and live with wider dialog with extra space. Patchset v3 review: P9 Center the data below the partition information graphic Didn't compile. Added needed #include <gtkmm/alignment.h> P10 Remove partition information vgname from status field Cosmetic. 1) Took the opportunity to reverse this small white space change making the conditions not line up in the else if condition here: @@ -298,24 +298,18 @@ void Dialog_Partition_Info::Display_Info() */ str_temp = _("Busy (At least one logical partition is m } - else if ( partition .filesystem == FS_LINUX_SWAP + else if ( partition .filesystem == FS_LINUX_SWAP || partition .filesystem == FS_LINUX_SWRAID 2) Changed the comment to the translators to consistently use the term LVM, rather than changing to logical volume manager for a single comment. $ egrep '\*.*LVM' Dialog_Partition_Info.cc * means that the partition is not yet a member of an LVM volume * means that the partition is a member of an LVM volume group... * The volume group has also been exported making the LVM phys... * means that the partition is a member of an LVM volume group... $ egrep '\*.*manager' Dialog_Partition_Info.cc * or logical volume manager physical volume These changes are incorporated into patchset v4. P6 Remove shadow border from partition information scrollable region Doesn't compile on CentOS 5.10 with this error: Dialog_Partition_Info.cc: In constructor ‘GParted::Dialog_Partition_Info::Dialog_Partition_Info(const GParted::Partition&)’: Dialog_Partition_Info.cc:58: error: ‘GTK_BIN’ was not declared in this scope Dialog_Partition_Info.cc:58: error: ‘gtk_bin_get_child’ was not declared in this scope Dialog_Partition_Info.cc:59: error: ‘GTK_VIEWPORT’ was not declared in this scope Dialog_Partition_Info.cc:59: error: ‘gtk_viewport_set_shadow_type’ was not declared in this scope make: *** [Dialog_Partition_Info.o] Error 1 Initial reaction is that some #includes for the GTK+ C API need adding. Guess that later gtkmm versions export the symbols, but not in gtkmm 2.10 included in CentOS 5.10. I'll continue to look into this. Everything else with the patchset is OK. Thanks, Mike
Created attachment 275207 [details] [review] Partition information warning not viewable patch (v5) Hi All, Here's patchset v5. Makes the following changes over v4: P1 Organize partition information fields into logical sections Remove #include <gtkmm/separator.h>. No longer needed as this patch removes use of the separater widget. P6 Remove shadow border from partition information scrollable region Adds #include <gtk/gtk.h> so that it compiles on CentOS 5.10. Successfully built every commit and tested on CentOS 5.10, CentOS 6.5 and Fedora 20. Works. I'm ready to commit this now. Mike
Hi Mike, Thanks for reviewing the patch set and catching the missing alignment.h include. I had that in my Dialog_Partition_Info.h but missed committing that change (I had it in the .h file. Having another pair of eyes and someone else testing helps catch these and other types of errors. :-) Last night I was thinking about the Utils::mk_label() alignment change since it has universal impact across GParted. I had forgotten to test dialogs such as Dialog_Partition_New.cc. From a quick test on kubuntu 12.04, I think that when the field label is paired with a value control (drop-down list, etc) that it looks better when vertically centered. I will perform some more testing regarding Utils::mk_label(). In the meantime please hold off on committing the changes. The change I am considering is to add a vertical alignment argument to Utils::mk_label() that defaults to ALIGN_CENTER (0.5), but can be set to ALIGN_TOP (0.0) for use with multi-line Dialog_Partition_Info fields. Curtis
Hi Curtis, If you want to use the symbolic constant ALIGN_START, rather than 0.0, I'm sure we can write some autoconf check and #define for gtkmm <= 2.22 which doesn't have it, to substitute ALIGN_TOP so this works: table->attach(*Utils::mk_label("<b>"+Glib::ustring(_("Members:"))+"</b>"), 1, 2, top, bottom, Gtk::Fill, Gtk::ALIGN_START); without having to do something ugly like: table->attach(*Utils::mk_label("<b>"+Glib::ustring(_("Members:"))+"</b>"), 1, 2, top, bottom, Gtk::Fill, #ifdef HAVE_ALIGN_START Gtk::ALIGN_START #else Gtk::ALIGN_TOP #endif ); Mike
Created attachment 275236 [details] [review] Partition information warning not viewable patch (v6) Hi Mike and Sinlu, Thank you Mike for fixing the spacing alignment of arguments in P10 (1) mentioned in comment #15. I just realized that my Eclipse editor was using the monospace 10 font that had the width of spaces just slightly smaller than characters such as letters and numbers. I've fixed that now. My bad. See: Eclipse code formatting is not aligning fields in columns properly https://stackoverflow.com/questions/13503309/eclipse-code-formatting-is-not-aligning-fields-in-columns-properly I've now changed to the Liberation Mono 10 font and that seems to be working correctly. :-) Following is a brief description of the patches that make up the new patch set v6: * Note that the default top vertical alignment P7 from patch set v5 has been deleted. P1 is cherry-picked from v5 P2 is cherry-picked from v5 P3 is cherry-picked from v5 P4 is cherry-picked from v5 P5 is cherry-picked from v5 P6 is cherry-picked from v5 P7 is P8 cherry-picked from v5 P8 is P9 cherry-picked from v5 P9 is P10 cherry-picked from v5 P10 Remove unused text_color argument from Utils::mk_label() method P11 Add optional yalign argument to Utils::mk_label() method P12 Set top vertical alignment for multi-line field and value label pairs The v6 patch set has been tested (both Partition_Info and Partition_New dialogs) with: Fluxbox on GParted Live 0.18.0-2 (Debian Live) GNOME on Fedora 20 KDE on Kubuntu 12.04 KDE on OpenSUSE 12.3 Unity on Ubuntu 13.04 From my testing this patch set is ready to be committed. Curtis EDIT: 'Just saw Mike's comment #18 about autoconf and ALIGN_START. I'm okay with passing the float value for now. I anticipate that we'll need to revisit this later when migrating the code base to gtkmm 3.x.
Created attachment 275239 [details] [review] Partition information warning not viewable patch (v7) Doh! 'Just noticed I had the wrong comment in P11 of patch set v6. I have now changed the P11 comment from: 0.5 /* ALIGN_TOP */ to: 0.5 /* ALIGN_CENTER */
Created attachment 275246 [details] [review] Partition information warning not viewable patch (v8) Hi Curtis, Patchset v7 tested on Kubuntu 12.04 LTS and CentOS 6.5. Passed review. I did however find another location in the code to apply (E) put LVM2 member names in a single string. In the Delete non-empty LVM2 PV dialog. I've added another patch to make this simple change. So patchset v8 is identical to v7 with the following addition: P13 Make LVM members selectable together in delete non-empty PV dialog I'm ready to apply this again, after you've looked at P13. Mike
Created attachment 275270 [details] [review] Partition information warning not viewable patch (v9) Hi Mike, Good catch with the member list in the delete LVM dialog in Win_GParted.cc. In an effort to make the members string list similar in both Dialog_Partition_Info.cc and Win_GParted.cc I edited P7 and P13 to use the same variable name "members_str", and to have both field heading and value use ALIGN_TOP. The reason I prefer to use ALIGN_TOP for both the field heading and the value is to ensure that the multi-line string is still top aligned if another longer multi-line string is added in the future. I successfully tested patch set v9 on Kubuntu 12.04 and Ubuntu 13.04. From my testing this patch set is ready to be committed. Curtis
Hi Curtis, I'm all for being consistent so I approve of changes in patchset v9. As such this patchset from comment #22 has been committed to the GIT repository for inclusion in the next release of GParted. The commits are: Organize partition information fields into logical sections (#690542) https://git.gnome.org/browse/gparted/commit/?id=7fc8aa49fe70b7c3d085d6fd592321f6a5e074ed Add partition information section titles and indent fields (#690542) https://git.gnome.org/browse/gparted/commit/?id=92c771bc7c9b17bcfa68ec2ab3a9b218c6d4da25 Organize partition information into two field & value columns (#690542) https://git.gnome.org/browse/gparted/commit/?id=898bc351980ef71f8c7bfc137712406b2829596d Place partition information percentages in their own column (#690542) https://git.gnome.org/browse/gparted/commit/?id=b271e367ea00bcb4a362ff578d08ee089d65ae0c Make the partition information dialog resizable (#690542) https://git.gnome.org/browse/gparted/commit/?id=ccaeb8dc516a2e02c40a99dfeb01581e9352bd94 Remove shadow border from partition information scrollable region (#690542) https://git.gnome.org/browse/gparted/commit/?id=960bb2c6cdfe70af11d49a47bc12237c7fcd70f6 Place partition information LVM2 member names in string list (#690542) https://git.gnome.org/browse/gparted/commit/?id=c0529abf363ed323c2f1ef1564e414f3b39517a8 Center the data below the partition information graphic (#690542) https://git.gnome.org/browse/gparted/commit/?id=c5f8220f606efcbd30081aa2040e4295cbdbf460 Remove partition information vgname from status field (#690542) https://git.gnome.org/browse/gparted/commit/?id=09711ae22d15cb96e2c02a7255f21bc85ef8b897 Remove unused text_color argument from Utils::mk_label() method https://git.gnome.org/browse/gparted/commit/?id=e075ab006ee51a70aeed66a161b2b15371b396da Add optional yalign argument to Utils::mk_label() method https://git.gnome.org/browse/gparted/commit/?id=6efa6234012b5fa112af4db08e0b89238d53981a Set top vertical alignment for multi-line field and value label pairs https://git.gnome.org/browse/gparted/commit/?id=72aa552469cadbad3bcf42e1861352ee3e067130 Make LVM members selectable together in delete non-empty PV dialog https://git.gnome.org/browse/gparted/commit/?id=960ce2df565c212debd3e47ee755fdb57c492e72 Thanks, Mike
This enhancement was included in the GParted 0.19.0 release on June 10, 2014.