Files

Merge lp://staging/~elementary-apps/pantheon-files/single-method-construct into lp://staging/~elementary-apps/pantheon-files/trunk

single-method-construct
Merge into trunk

Proposed by Danielle Foré on 2016-06-29

Status:

Merged

Approved by:

Danielle Foré on 2016-07-03

Approved revision:

2232

Merged at revision:

2230

Proposed branch:

lp://staging/~elementary-apps/pantheon-files/single-method-construct

Merge into:

lp://staging/~elementary-apps/pantheon-files/trunk

Diff against target:

436 lines (+148/-131)

1 file modified

src/View/PropertiesWindow.vala (+148/-131)

To merge this branch:

bzr merge lp://staging/~elementary-apps/pantheon-files/single-method-construct

Low

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Felipe Escoto (community)	third set of eyes	2016-06-29	Approve on 2016-07-03
Review via email: mp+298686@code.staging.launchpad.net

Commit message

PropertiesWindow.vala:
* Remove get_info method and construct info widgets in construct_info_panel
* separate complex info-fetching logic into small methods independent from widget construction
* Set resolution_value directly since it now has its own variable
* Ellipsize location_value
* Remove unused variables and declarations
* Rename variables for value and key labels for clarity

Description of the change

Instead of constructing the info panel in two methods (get_info and construct_info_panel), we move the construction into the single construct info panel

Also, we move the information-fetching logic into separate methods. This allows us to clearly see our conditionals and understand how things are being built

The score on this one is gonna be lame. Gotta add lots of lines since we're not sharing variables

Revision history for this message

Jeremy Wootten (jeremywootten) wrote on 2016-06-30:

I suggest moving the filename manipulation stuff into PF.FileUtils. I suspect there may be more elegant ways of doing it as well.

Revision history for this message

Jeremy Wootten (jeremywootten) wrote on 2016-06-30:

private class Pair no longer needed.

Some labels are declared as Gtk.Labels but constructed as KeyLabels or ValueLabels; others are declared as KeyLabels or ValueLabels.

The classes are not indented within the namespace - is this intentional?

AbstractPropertiesDialog declares several independent classes; normally it is preferred to have one class per file otherwise they are difficult to find; moreover use of KeyLabel etc without a qualifier could cause confusion with a GLib object. One solution would be to move them inside the AbstractPropertiesDialog class and then refer to them as AbstractPropertiesDialog.KeyLabel outside that file. The whole question of namespaces and folder structure probably needs addressing anyway.

ImgEventBox should not really be in the Granite.Widgets namespace? It is not defined in the Granite source tree now.

Revision history for this message

Danielle Foré (danrabbit) wrote on 2016-07-01:

Good catch!

Oops, you're right. Fixed.

I didn't touch that in this diff, so I'd prefer to change that in another diff. I was thinking of doing a branch to try to organize a little better. They're currently in the Marlin.View namespace, I was thinking of creating a Files.Widgets namespace and moving them into a Labels.vala or something. Do you think it's worth having 2 files (ValueLabel.vala and KeyLabel.vala) since they're such tiny classes?

You're right it shouldn't be, but it looks like it is in Granite.Widgets namespace in pantheon-files/libwidgets/Chrome/ImgEventBox.vala

lp://staging/~elementary-apps/pantheon-files/single-method-construct updated on 2016-07-01

2230. By Danielle Foré on 2016-07-01: extra space

Revision history for this message

Jeremy Wootten (jeremywootten) wrote on 2016-07-02:

The elementary code-style document does recommend one file per class but I guess it is not mandatory. Another option would be to have a single class with three construction methods:
new PropertiesLabel.header (string txt)
new PropertiesLabel.key (string txt)
new PropertiesLabel.value (string txt)

I agree the namespace stuff can be left for another branch.

lp://staging/~elementary-apps/pantheon-files/single-method-construct updated on 2016-07-02