Created attachment 292589 [details] [review]
patch :)

Thank you Michael for your interest in GParted and providing a patch.

> We may need to clarify the difference between Name and Label
> in the UI somehow.

This is a good point.  I have not looked at your patch yet, but wanted to point out two other bug reports in this area.

Bug 690953 - Partition name support

Bug 691681 - Improve Partition Info dialog to be Partition properties dialog

The last direction discussed is mentioned in:
Bug 691681 Comment 133:
https://bugzilla.gnome.org/show_bug.cgi?id=691681#c133

Curtis

I guess my patch is useless then :P

Hi Michael,

I think Curtis is merely referencing other work in the same area and not
declining your patch.

Curtis,

I've only glanced at this code and given it a quick test, but I like the
approach.  It basically copies how file system labelling works for
partition naming and adds a column to the partition list view.

Bug 690953 - only has a small amount of code to add read-only gpt
             partition name support

Bug 691681 - tried to do too much in redesigning partition properties
             and ground to a standstill

If you are happy with this approach I will work with Michael to code
commitable.  Initial thoughts are code and UI renaming so we distinguish
FS Labels from PTN Names, add GParted manual update and order columns in
the partition list view as:
Partition | PTN Name | File System | FS Label | Size ...

Thanks,
Mike

Hi Mike,

Thanks for investigating this patch and reviewing the other two bugs.  Your summary is accurate, and I fear that I discouraged the previous developer while we went through many iterations of partition properties.  In hindsight it might have been better to proceed with the read-only partition name support.  With that being said, I learned a lot while we went through the process of redesigning partition properties.  And with that knowledge I do not wish to hinder the partition name enhancement again.

Your suggested approach and ordering of columns sounds good to me.  Please proceed.


Michael,

Mike's comment is correct.  I wished to point out previous work, so that we might avoid some of the problems encountered last time.  Your posting of this patch shows initiative to identify a need, learn the existing code, develop a solution, and post the code for review.

If you haven't already done so, it might be a good idea to read the GParted development page to see how we manage development, especially the releases section.
http://gparted.org/development.php

Curtis

ok, so anything about code style etc that would need improvements?
I didn't add any translation files yet, do we need to create these now or are there translators for that?

Also, I found a small bug. When u create a partition, u can't change the name until you apply the changes because. I guess there's some problem with "ped_disk_type_check_feature" when using temporary partitions.

Hi Curtis,


UI changes:

Can you validate the details in my UI changes below.  It seems necessary
to change "Partition Label" to "File System Label" everywhere and add
"Partition Name" to keep it clear to the users what these operations
will perform.  I had a read through the GNOME Translation Project's
"L10N Guidelines for Developers", and in particular the "Use clear,
simple and consistent language" page:
https://wiki.gnome.org/TranslationProject/DevGuidelines/Use%20clear%2C%20simple%20and%20consistent%20language

1) We need to make it clear to the user that it's labelling the file
   system and naming the partition.  To this end the following changes
   should be made:

1.1) In the main window, order columns as:
     Partition | Name | File System | Mount Point | Label | ...
     [Already agreed]
     "Partition Name" and "File System Label" just seem too long from a
     layout point of view, but I am open to persuasion.

1.2) Rename and reorder the partition menu items to:
     -----------------
     Name Partition
     Manage Flags
     Check
     Label File System
     New UUID
     -----------------

1.3) Change existing dialog title to "Set file system label on %1"

1.4) Change existing operation description to "Set file system label .."


Code renaming:

Should we also rename any of the internal code; constants, functions,
classes and files dealing with labelling file systems?  Such as:

    OPERATION_LABEL_PARTITION     -> OPERATION_LABEL_FILESYSTEM
    Partition::label_known()      -> Partition::filesystem_label_known()
    Partition::get_label()        -> Partition::get_filesystem_label()
    Partition::set_label()        -> Partition::set_filesystem_label()
    class Dialog_Partition_Label  -> class Dialog_FileSystem_Label
    class OperationLabelPartition -> class OperationLabelFileSystem
    Dialog_Partition_Label.cc     -> Dialog_FileSystem_Label.cc
    OperationLabelPartition.cc    -> OperationLabelFileSystem.cc

I think the code should match the user concepts as much as possible so
yes.


Michael,

As I am asking you to make what appears to be a lot of extra changes I
will help you with any of it that you wish.  Will provide more details
when we have Curtis' view.

Thanks,
Mike

"lot of extra changes". These are just minor things like column orders and names :P

I'll upload a new patch as soon as Curtis confirmed ur concept.

Mike and Michael,

Thank you for taking the time to review this change while keeping consistency in mind and alignment with the GNOME HIG.  I agree with the proposed changes in comment 7.  Please proceed.

Curtis

Hi Michael,

Just to flesh out what I mean by a "lot of extra changes".  This is what
I will help with if you want.

1) New patch to apply before your current patch which renames existing
   label identifiers and source as outlined above in comment #7.  Will
   almost certainly have knock on context changes to your initial patch.

2) GParted Manual update as a third patch.

Can you also set your real name in your git email address please.  It is
currently: M1cha <...>

Thanks,
Mike

More review details.


Translation files:

2) Just add new source files which contain translatable strings to
   po/POTFILES.in.  The GNOME Translation Teams take care of the reset.
   https://l10n.gnome.org/module/gparted/
      Dialog_Partition_Name.cc
      OperationNamePartition.cc


Code formatting:

3) Current code is inconsistent.  Match it the best you can with the
   these extra hints:
   * No space before trailing semicolon
   * No spaces in the middle of a compound identifier
   * Use "Smart Tabs" indentation.  Tabs for indentation, spaces for
     alignment.
     http://www.emacswiki.org/SmartTabs

4) Applying your patch with git am reports a number of trailing
   white space errors.  Mostly looks like white space errors from the
   original code you copied and then amended.
       warning: squelched 7 whitespace errors
       warning: 12 lines add whitespace errors.


Additional UI item:

1.5) Change partition name dialog limits the name length to the maximum
     length of the file system the partition contains.  GPT limits it
     to 36 characters.  Use a symbolic constant.


Code review:

To follow later.


Mike

Created attachment 292749 [details] [review]
rename 'Label' to 'FileSystem Label' in preparation for partition names

Mike: "3) Current code is inconsistent.  Match it the best you can with the"
I copied the code from the Label feature and always checked the surrounding code, so wouldn't it be inconsistent to do what u suggested?

Created attachment 293126 [details] [review]
Rename label_partition to label_filesystem (v2)

Hi Michael,


On review of the your rename patch from comment #12 I noticed a few
issues:

1) The commit message doesn't really say why the change is necessary.

2) It changes AUTHORS, NEWS, ChangeLog which document previous history.
   These should be left alone.

3) It changes comments in the translation files in po/*.po.  Developers
   don't change translation files.  They are exclusively changed via the
   GNOME Translation project.


Given that this bug report is more than a trivial change I felt that it
would be better to write the rename label patch myself to show you how
we prefer patches, rather than go through too many back and forth
discussions.  Notable points:

A) Commit messages explain why the change is being done.

B) Multiple smaller commits which are easier for simple minded reviewers
   to understand and git bisect works wonders with.

(The best I can tell this is how all open source projects prefer their
changes).

C) Format commit message how we prefer; with bug number in the summary
   line and bug text as a final footer.


Please rebase your "Add support for GPT partition name" on top of this
patchset.  Making two patches, first adding read-only support of
partition names, and the second adding read-write support would be nice,
but not required.


Further questions for thought:

Q1) Is this intended for GPT only or all partition types which libparted
    supports naming for: gpt, pc98, amiga, mac, dvh?  Should it be
    restricted to GPTs only or not?

Q2) What is the maximum character size for the names of each partition
    type?  (Solving this can easily be a separate patch).


Thanks,
Mike

Created attachment 295507 [details] [review]
main patch

Q1) I don't know, before I started working on this patch I didn't even know there is anything else but gpt and mbr :D

Q2) According to wikipedia: "72 bytes	Partition name (36 UTF-16LE code units)"

I didn't split the patch int ro and rw support, because I currently don't have much time for this, sry. But I think the patch is small enough already.

Hi Michael,

Just to let you know that I am reviewing this.

Thanks,
Mike

Hi Curtis,

When copying existing source files to create new code and only making
minor changes do you know that we should do in terms of the copyright
line(s)?  At the moment Michael has left your name at the top of the new
files.  E.g.:

diff --git a/include/Dialog_Partition_Name.h b/include/Dialog_Partition_Name.h
new file mode 100644
index 0000000..bfec0f7
--- /dev/null
+++ b/include/Dialog_Partition_Name.h
@@ -0,0 +1,49 @@
+/* Copyright (C) 2008 Curtis Gedak
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by

Thanks,
Mike

Hi Mike,

Technically I think that anyone who makes a significant change to a file (say more than 15 lines) becomes one of the owners of copyright in the code.  However, I am not a lawyer so my understanding might not be correct.

My rule of thumb is that if the file is new then the Copyright line should list the person who created the file and the year in which it was created.

Curtis

Hi Michael,

I've been through the code now.  It's pretty good.  Tested it and it
works fine.  Few things I think should be changed:

1) Currently carry around the boolean identifying naming support at the
   partition level, but it's a partition table property.  Should be
   moved to the Device object.  Device object is available where the
   check is needed.

2) As the code is only supporting GPT it should restrict to only working
   with GPT partitions, otherwise different name lengths should be
   handled.

Minor stuff:
Copyright line update, #define BORDER is redundant, trivial white space,
comment GPT partition name length approximation as 36 chars is not
always exactly the same as 36 UTF-16LE code units.

So you said you don't have much time, so I'll make these changes for
you.

Mike

Hi Michael and Curtis,

Question: When copying and pasting an existing partition/file system
into either an existing partition or into empty space (creating a new
partition) do you think the partition name should be copied too?

I'm thinking not.

Thoughts welcome,
Mike

depends on how this is presented in the UI.
"Copy/Move partition" should copy the name, too.
"Copy Partition Data" shouldn't.

Also I'm not sure what happens if two partitions have the same name.

It's just called Copy then Paste in the Partition menu.  However some
operations clearly operate at the file system level only, such as:
  Mount, Unmount, Swapon, Swapoff ...
  Label File System
  Check
  New UUID
Where as others work at the partition level:
  Resize/Move
  Manage Flags
and others work on both parts:
  Format
  New
    Both update partition type appropriately for the created file system
    type.

Nothing in desktop linux will care if two partitions have the same name.
Everything works from device names, file system UUIDs or file system
labels.  I have no idea what Android user space will do.  I assume
partition names are only for the benefit of users so guess Android won't
care about duplicates.

Still think not copying partition names in a copy operation is the way
to go.

In Android, UUID's and device names aren't used at all, so I was concerned about kernel(there are /sys nodes named by gpt labels) and bootloader(they choose the partition by name).

I just tried it and it works just fine. The bootloader just chooses the first partition with the name, and linux does the same when creating the named symlinks.

If you decide to not copy the names, that's fine with me.

(In reply to comment #21)
> Question: When copying and pasting an existing partition/file system
> into either an existing partition or into empty space (creating a new
> partition) do you think the partition name should be copied too?
> 
> I'm thinking not.

Agreed.

Currently we only copy the file system details (basically the partition contents) to the new partition.  Partition properties, such as flags, are not copied.  One challenge that arises if we copied flags is what to do with a partition containing the boot flag because there can only be one boot flag in the partition table.  The current GParted code does not copy the flags, and I don't think it should copy the partition name either.

Curtis

Created attachment 295857 [details] [review]
GPT partition name support (v4)

Here's the final patchset.

It's got my rename set from comment 14, Michael's add GPT naming patch
from  comment 15 with discussed changes and updates to the GParted
Manual.

Curtis,

Can you review the GParted Manual updates please.  Then I'll be ready to
push upstream.

Thanks,

Hi Mike,

I will review and test these updates.  If you are comfortable with these as they currently exist, then if I don't see any problems I will push these upstream.

Curtis

Thank you both for working on this GPT partition name enhancement.

Mike,

While reviewing the patch set in comment #26 I found a few minor
issues.  Hence I made a list of potential areas to consider for the
patch set.

Following are my review comments.  Please feel free to question any
comments I've made.


[03/10] Rename member variables and methods in Partition class

    Minor change in commit message from:

      label_label   -> have_filesystem_label

    to:

      have_label    -> have_filesystem_label


[04/10] Rename methods in GParted_Core and Win_GParted classes

    Minor change in commit message from:

        allow_label_partition()    -> allow_label_partition()

    to:

        allow_label_partition()    -> allow_label_filesystem()


[06/10] Rename class to Dialog_FileSystem_Label

    Minor change in commit message from:

      class Dialog_Partition_Label   -> Dialog_Partition_Label
      file Dialog_Partition_Label.h  -> Dialog_Partition_Label.h
      file Dialog_Partition_Label.cc -> Dialog_Partition_Label.cc

    to:

      class Dialog_Partition_Label   -> Dialog_FileSystem_Label
      file Dialog_Partition_Label.h  -> Dialog_FileSystem_Label.h
      file Dialog_Partition_Label.cc -> Dialog_FileSystem_Label.cc


[07/10] Add support for GPT partition names

    I think we should expand upon the short commit message since this
    patch adds functionality that isn't limited to Android usage only.
    Perhaps mention difference between partition names and file system
    labels.


    Typo in src/Dialog_Partition_name.cc:

      "gurantee" should be "guarantee"


    Code doesn't exactly match surrounding code:

      This might be a minor point but I noticed things like the following:

      copied_partition .messages .clear() ;
      copied_partition .clear_mountpoints() ;
+     copied_partition.name.clear();           <-- spacing is different


    Automated "make distcheck" fails with:

      ../../src/Dialog_Partition_Name.cc:17:46: fatal error:
      ../include/Dialog_Partition_Name.h: No such file or directory
      compilation terminated.

      It appears that "Dialog_Partition_Name.h" is missing from
      EXTRA_DIST variable in include/Makefile.in

      Similarly with "OperationNamePartition.h"


[09/10] Use file system label terminology in the Manual

    Perhaps in renaming "Partition Label" to "File System Label" we
    should ignore the partition component.  I'm thinking ahead to when
    unpartitioned device support is added.

    What will happen to "File System Label"?
    Will it work on both partitioned and unpartitioned devices?
    If so then we should not mention "Partition" in the documentation
    for File System Label.  That way we won't need to remove it later.


Another future thought - Do we want to enhance Dialog_Partition_New to
include a partition name field when operating on a device with GPT?
This is an enhancement that can be considered at a later date.  There
is lots on the go right now.  :-)


Curtis

Created attachment 295885 [details] [review]
GPT partition name support (v5)

Hi Curtis,

Thanks for catching my mistakes.  I've changed everything you've
suggested except I've left the GParted Manual as is.

Strong opinion following ...
I'm a firm believer in strong boundaries to the scope of changes.  A
patchset should change EVERYTHING that is necessary for the current
change and NO MORE.

As such I think the change to the Manual exactly fits and is consistent
with the rest of the Manual as it talks about partitions everywhere and
make just enough distinction for labels of file systems within
partitions.  Quite a few operations which currently are described as
partition operations only make file system changes such as check and new
UUID.  Bug 743181 "add unpartitioned drive read-write support" should
make all the changes it needs to the Manual without bringing a bit of it
forward to this change.

Good suggestion to add partition name field to Dialog_Partition_New.
Definitely going to leave it as an idea for later though.

Thanks,
Mike

Question for Michael Zimmermann:

First, thank you for working on this enhancement with Mike.  I anticipate that GParted users will be happy to see the new functionality.  :-)

Since you've made a significant contribution (more than 15 lines of code) to GParted, we would like to provide recognition for your effort.  This is done by adding your name and email address to the AUTHORS file, and to the credits section of Help -> About.

https://git.gnome.org/browse/gparted/tree/AUTHORS?id=GPARTED_0_21_0
https://git.gnome.org/browse/gparted/tree/src/Win_GParted.cc?id=GPARTED_0_21_0#n1469

Would it be okay if we add your name and email address in both these locations to show credit for your work?

Curtis


PS for Mike Fleetwood.

Thanks for your work with Michael to refactor the code and get this patch set production ready.  Your explanation of why you would like the reference to partition in the manual for file system labels rings true for me.  I will review the patch set one more time before committing this enhancement.

Curtis: Ofc, thx :)
When will the next LiveCD get built? This patch is needed to change the Partition table of phones and Windows users would be very happy to be able to use this tool.

Thanks Michael for the quick response.  I will provide credit for your work in the two aforementioned locations.

Regarding Live image creation, we just made a release on Jan 27, 2015 and do not plan on a new release for at least a month or so.  Each release takes a fair amount of effort.  In addition Mike has invested lots of effort in Bug 743181 - "Add unpartitioned drive read-write support", and we would like to include that in the next release as well.

Curtis

The patch set in comment #29 has been committed to the git repository for inclusion in the next release of GParted.

The relevant git commits can be viewed at the following links:

Rename Partition Label to File System Label in the GUI (#741424)
https://git.gnome.org/browse/gparted/commit/?id=332a2e7a79048b06bedb707d40d98e5f44d3f5e0

Rename enum to OPERATION_LABEL_FILESYSTEM (#741424)
https://git.gnome.org/browse/gparted/commit/?id=d480800600be714a0e706b51a147d329d8280e52

Rename member variables and methods in Partition class (#741424)
https://git.gnome.org/browse/gparted/commit/?id=63aeb150ace06f678241f514c550e51b22d2f823

Rename methods in GParted_Core and Win_GParted classes (#741424)
https://git.gnome.org/browse/gparted/commit/?id=3630b9c83b271c35e165b024d1a237e33461ef6c

Rename class to OperationLabelFileSystem (#741424)
https://git.gnome.org/browse/gparted/commit/?id=04450c577c94ce8ee9998220c6b2ab4e2dfbe77e

Rename class to Dialog_FileSystem_Label (#741424)
https://git.gnome.org/browse/gparted/commit/?id=e1dc89cd117b55ec4fbc53212da568f074b725ac

Remove redundant second if condition in Display_Info()
https://git.gnome.org/browse/gparted/commit/?id=b278782ef62c5a9aa75d5bab3ee638e12d33fbc8

Add support for GPT partition names (#741424)
https://git.gnome.org/browse/gparted/commit/?id=1f5841b4adc44be7f41d7a3e155774a464a49d2b

Use file system label terminology in the Manual (#741424)
https://git.gnome.org/browse/gparted/commit/?id=becbd7ac85a8d273d488ff3ddd9ef0497806a536

Describe the name partition operation in the Manual (#741424)
https://git.gnome.org/browse/gparted/commit/?id=64d569cfb2696e5798ec7c7d416f92dc24be853a

Thanks goes to the following people for their efforts to add support for GPT partition names:

  Michael Zimmermann
  Sinlu Bes
  Mike Fleetwood

Sincerely,
Curtis

*** Bug 690953 has been marked as a duplicate of this bug. ***

*** Bug 691681 has been marked as a duplicate of this bug. ***

This enhancement was included in the GParted 0.22.0 release on March 23, 2015.