Add detail page for firmware releases #182

meisenzahl · 2021-01-26T17:40:08Z

Closes #172

meisenzahl · 2021-01-26T19:38:17Z

Current state

I am so bad at doing layouts 🙈

@cassidyjames I could use some help 🙏

cassidyjames

The actual contents of the detail page looks close, and I can probably do any small layout cleanup once we're closer. I have a couple of thoughts though:

Formatter seems interesting; I would think some if not all of these would be built into GLib or something and not require manual handling here. Dealing with bytes and seconds and XML all seem pretty common. 😛 So that might be worth looking into instead of reinventing it here.

Layout-wise, as I mentioned in Slack, ideally the detail page would be in a Hdy.Deck and within the same container as the list (so styled with the white background and border and everything); this way it feels like a page within that list, instead of not really having a clear container. See the Installer and Initial Setup lists where we use Hdy.Deck for an example of that (the right half of this view):

meisenzahl · 2021-01-26T21:05:36Z

@cassidyjames layout-wise like this?

cassidyjames · 2021-01-26T21:06:55Z

@meisenzahl yeah that's much closer! Looks like we might need a style class or something on that top bar (see: Installer/Initial Setup to match)?

meisenzahl · 2021-01-26T21:17:06Z

Based on https://github.com/elementary/initial-setup/blob/master/src/Widgets/VariantWidget.vala

cassidyjames

Just a couple of style things to better match the other places we use this type of layout

src/Views/FirmwareReleaseView.vala

Co-authored-by: Cassidy James Blaede <[email protected]>

* FirmwareReleaseView: Layout tweaks * Revert whitespace change * Tweak content spacing * Tweak margin * More spacing tweaks * Center up the key/vals in their own grid * De-prioritize key/vals

meisenzahl · 2021-01-27T17:31:15Z

Formatter seems interesting; I would think some if not all of these would be built into GLib or something and not require manual handling here. Dealing with bytes and seconds and XML all seem pretty common. 😛 So that might be worth looking into instead of reinventing it here.

@cassidyjames I refactored the code to use GLib.format_size to pretty print Bytes. But I could not find a function to pretty print time.

Regarding XML: I could add libxml2-dev as a dependency. Is there anything against it?

…ementary/switchboard-plug-about into add-detail-page-for-firmware-releases

danirabbit

I haven't read through all the code yet, but noticed this thing

src/Views/FirmwareView.vala

meisenzahl · 2021-02-09T05:58:14Z

Current state

TODO

Add placeholder for devices without releases in FirmwareReleaseView
Hide button in header_box if not upgradable or latest release is installed

meisenzahl · 2021-02-15T14:46:08Z

Peek.2021-02-15.15-33.mp4

Should all be working now 😀

danirabbit · 2021-02-16T18:07:25Z

I think I would prefer if we merged #202 first so that we're not making it harder to port to libfwupd

danirabbit · 2021-02-26T20:31:39Z

We have fwupd docs now on Valadoc :) https://valadoc.org/fwupd/Fwupd.html

meisenzahl · 2021-02-27T14:39:24Z

The way libfwupd's logic works, no release information is now displayed after a updatable device is updated to it's latest release...

Compare with my last video for Unifying Receiver.

danirabbit

Just stuff I noticed on a first pass here

danirabbit · 2021-03-02T21:51:03Z

src/Views/FirmwareReleaseView.vala

+        add (content);
+
+        back_button.clicked.connect (() => {
+            back ();


Since the deck is an ancestor of this, we can use Gtk.Widget.get_ancestor and avoid sending signals up:

Suggested change

back ();

var deck = (Hdy.Deck) get_ancestor (typeof (Hdy.Deck));

deck.navigate (Hdy.NavigationDirection.BACK);

danirabbit · 2021-03-02T21:55:18Z

src/Views/FirmwareReleaseView.vala

+                placeholder.icon_name = icons.data[0];
+            }
+
+            content.visible_child_name = "placeholder";


It's faster to compare refs than strings and placeholder is already available so better to do:

Suggested change

content.visible_child_name = "placeholder";

content.visible_child = placeholder;

danirabbit · 2021-03-02T22:16:50Z

src/Views/FirmwareView.vala

+            firmware_release_view.update.connect ((device, release) => {
+                on_update_start (device);
+
+                update.begin (device, release, (obj, res) => {
+                    update.end (res);
+                    on_update_end ();
+                });
+            });


This should be connected when the firmware release view is constructed, otherwise you're creating a new signal connection every time the release view is shown

danirabbit · 2021-03-02T22:18:19Z

src/Views/FirmwareView.vala

+                    progress_alert_view.title = _("“%s” is being updated").printf (device.get_name ());
+                    visible_child = progress_view;


It looks like you created a function for this, but then didn't use it here?

danirabbit · 2021-03-02T22:40:33Z

src/Widgets/FirmwareUpdateRow.vala

+    public bool is_updatable {
+        get {
+            return release != null && device.get_version () != release.get_version ();
+        }
+    }


Why was this done? We already check this on construct, this change means we have to run this comparison every time this property is used

danirabbit

There's a lot going on here with so many things moving up into the FirmwareView, I wonder if it would make sense to split this into two PRs: one dealing with only the refactoring of the update logic and a separate one for adding the release view

meisenzahl · 2021-03-07T15:59:40Z

@danrabbit sure let's break this up into separate PRs 👍

meisenzahl added 6 commits January 26, 2021 18:39

Add initial detail page

cb886db

Add all fields to detail page

d4688b3

Move update logic to FirmwareView

72ecac9

Slightly improve layout

ee24554

Add support for ordered lists

ab03174

Add button only if update is available

68a39b3

meisenzahl marked this pull request as ready for review January 26, 2021 19:38

meisenzahl requested a review from cassidyjames January 26, 2021 19:38

cassidyjames suggested changes Jan 26, 2021

View reviewed changes

Use Hdy.Deck

e587752

Update layout

b9a2ba0

meisenzahl added 2 commits January 26, 2021 22:38

Update FirmwareReleaseView instead of destroying it

4026eef

Use insensitive button when up to date

554cf45

cassidyjames suggested changes Jan 26, 2021

View reviewed changes

src/Views/FirmwareReleaseView.vala Outdated Show resolved Hide resolved

src/Views/FirmwareReleaseView.vala Outdated Show resolved Hide resolved

src/Views/FirmwareReleaseView.vala Outdated Show resolved Hide resolved

meisenzahl and others added 6 commits January 26, 2021 22:41

Update src/Views/FirmwareReleaseView.vala

10d4950

Co-authored-by: Cassidy James Blaede <[email protected]>

Update src/Views/FirmwareReleaseView.vala

604a63d

Co-authored-by: Cassidy James Blaede <[email protected]>

Update src/Views/FirmwareReleaseView.vala

c29fed4

Co-authored-by: Cassidy James Blaede <[email protected]>

FirmwareReleaseView: Layout tweaks (#184)

6d2a767

* FirmwareReleaseView: Layout tweaks * Revert whitespace change * Tweak content spacing * Tweak margin * More spacing tweaks * Center up the key/vals in their own grid * De-prioritize key/vals

Merge branch 'master' into add-detail-page-for-firmware-releases

a1904aa

Refactor to use GLib.format_size

9d2f04c

meisenzahl and others added 5 commits January 27, 2021 18:33

Merge branch 'master' into add-detail-page-for-firmware-releases

4623e35

Merge branch 'master' into add-detail-page-for-firmware-releases

1f23286

Merge branch 'master' into add-detail-page-for-firmware-releases

12b4e58

Use AppStream to convert markup

fba1eb3

Merge branch 'add-detail-page-for-firmware-releases' of github.com:el…

e528af9

…ementary/switchboard-plug-about into add-detail-page-for-firmware-releases

danirabbit requested changes Feb 6, 2021

View reviewed changes

src/Views/FirmwareView.vala Outdated Show resolved Hide resolved

Only connect once

9014e90

meisenzahl dismissed cassidyjames’s stale review via 9014e90 February 6, 2021 11:03

meisenzahl and others added 2 commits February 6, 2021 12:04

Merge branch 'master' into add-detail-page-for-firmware-releases

6b5028d

Merge branch 'master' into add-detail-page-for-firmware-releases

b6b1881

cassidyjames and others added 5 commits February 9, 2021 11:38

Merge branch 'master' into add-detail-page-for-firmware-releases

90c7fe5

Merge branch 'master' into add-detail-page-for-firmware-releases

953eccb

Add placeholder and hide update button

86db07a

Merge branch 'master' into add-detail-page-for-firmware-releases

d17413f

Fix title

2775c7c

meisenzahl requested a review from danirabbit February 15, 2021 14:46

Make nullable

4bca8b3

meisenzahl added 7 commits February 27, 2021 13:34

Merge branch 'master' into add-detail-page-for-firmware-releases

e4f8051

Remove appstream as dependency

6337491

Make it buildable again...

ec60751

Readd and use appstream

33f5c0a

Fix logic

8818ee8

Fix whitespace

85b85a4

Add dynamic check if device is updatable

a106ab0

danirabbit requested changes Mar 2, 2021

View reviewed changes

danirabbit reviewed Mar 2, 2021

View reviewed changes

meisenzahl closed this Mar 7, 2021

meisenzahl mentioned this pull request Mar 10, 2021

Add detail page for firmware releases - Revenge of libfwupd #206

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add detail page for firmware releases #182

Add detail page for firmware releases #182

meisenzahl commented Jan 26, 2021

meisenzahl commented Jan 26, 2021

cassidyjames left a comment •

edited

Loading

meisenzahl commented Jan 26, 2021

cassidyjames commented Jan 26, 2021

meisenzahl commented Jan 26, 2021

cassidyjames left a comment

meisenzahl commented Jan 27, 2021

danirabbit left a comment

meisenzahl commented Feb 9, 2021 •

edited

Loading

meisenzahl commented Feb 15, 2021

danirabbit commented Feb 16, 2021

danirabbit commented Feb 26, 2021

meisenzahl commented Feb 27, 2021

danirabbit left a comment

danirabbit Mar 2, 2021

danirabbit Mar 2, 2021

danirabbit Mar 2, 2021

danirabbit Mar 2, 2021

danirabbit Mar 2, 2021

danirabbit left a comment

meisenzahl commented Mar 7, 2021

	back ();
	var deck = (Hdy.Deck) get_ancestor (typeof (Hdy.Deck));
	deck.navigate (Hdy.NavigationDirection.BACK);

	content.visible_child_name = "placeholder";
	content.visible_child = placeholder;

		progress_alert_view.title = _("“%s” is being updated").printf (device.get_name ());
		visible_child = progress_view;

Add detail page for firmware releases #182

Add detail page for firmware releases #182

Conversation

meisenzahl commented Jan 26, 2021

meisenzahl commented Jan 26, 2021

Current state

cassidyjames left a comment • edited Loading

Choose a reason for hiding this comment

meisenzahl commented Jan 26, 2021

cassidyjames commented Jan 26, 2021

meisenzahl commented Jan 26, 2021

cassidyjames left a comment

Choose a reason for hiding this comment

meisenzahl commented Jan 27, 2021

danirabbit left a comment

Choose a reason for hiding this comment

meisenzahl commented Feb 9, 2021 • edited Loading

Current state

TODO

meisenzahl commented Feb 15, 2021

danirabbit commented Feb 16, 2021

danirabbit commented Feb 26, 2021

meisenzahl commented Feb 27, 2021

danirabbit left a comment

Choose a reason for hiding this comment

danirabbit Mar 2, 2021

Choose a reason for hiding this comment

danirabbit Mar 2, 2021

Choose a reason for hiding this comment

danirabbit Mar 2, 2021

Choose a reason for hiding this comment

danirabbit Mar 2, 2021

Choose a reason for hiding this comment

danirabbit Mar 2, 2021

Choose a reason for hiding this comment

danirabbit left a comment

Choose a reason for hiding this comment

meisenzahl commented Mar 7, 2021

cassidyjames left a comment •

edited

Loading

meisenzahl commented Feb 9, 2021 •

edited

Loading