Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New Battery health section #194

Closed
wants to merge 97 commits into from
Closed

Conversation

andirsun
Copy link
Contributor

@andirsun andirsun commented Jul 28, 2021

fixes: #156
fixes: #178
I am going to put all current settings in Behavior page and create a new Battery Health info page as disccused with @cassidyjames here #156 (comment)

Preview:

Screenshot from 2021-08-17 13 19 32

Rechargeable devices

Screenshot from 2021-08-16 11 30 06

Non-rechargeable devices

Screenshot from 2021-08-16 11 30 08

@andirsun andirsun changed the title (WIP) New side bar and Battery Health section New side bar Jul 28, 2021
@andirsun andirsun marked this pull request as ready for review July 28, 2021 04:43
@cassidyjames
Copy link
Contributor

cassidyjames commented Jul 28, 2021

@andirsun typically I'd say iterative is best here, but changing to a sidebar doesn't really make sense on its own without adding a second view… so in this case I think I'd be fine to do the sidebar and battery health stuff all in one PR.

You could do any code cleanup/bug fixes you find along the way as iterative PRs against master, though.

@andirsun andirsun changed the title New side bar New Battery health section Jul 28, 2021
@andirsun andirsun marked this pull request as draft July 28, 2021 16:38
src/Plug.vala Outdated Show resolved Hide resolved
@andirsun
Copy link
Contributor Author

@cassidyjames How can I edit the Title of Granite.SimpleSettingsPage without modify the title in the side bar.
I want to Set the tittle Built-in Battery but in the side bar only show Built-in

@cassidyjames
Copy link
Contributor

cassidyjames commented Jul 28, 2021

@andirsun they’re the same with SimpleSettingsPage, so it would probably be best to just use the full name “Built-in battery”. Alternatively you could use a SettingsPage and create the top of the view’s layout manually, but that seems unnecessary.

@andirsun andirsun marked this pull request as ready for review July 28, 2021 23:39
@andirsun andirsun requested a review from cassidyjames July 28, 2021 23:39
@andirsun
Copy link
Contributor Author

@cassidyjames Revert change Done !

@andirsun
Copy link
Contributor Author

@JoseExposito Could you please test Again if

I was going to open a bug report, but it looks like the issue is not present in master. I'm on a desktop, so there are no administrator actions available for me, however, the administrator banner is displayed:

image

@JoseExposito Could you please test this again ?

@andirsun
Copy link
Contributor Author

Have we removed the "Devices" section in the sidebar?

image

With unknown values, the design looks a bit off, specially the bigger health section and the progress bar being smaller than the text. Something to consider is that in some languages -for example, in Spanish unknown is "Desconocido"- the string is going to be longer.

It is not visible in the screenshot, but the charge progress bar shows an animation. I looks like my device is charging, but it's not.

Code looks great, really happy to see how well this is shaping up!

@JoseExposito And Could your test this again ?

@JoseExposito
Copy link
Member

The admin banner is no longer displayed 🎉 This is how the detail view looks like now:

image

@andirsun andirsun requested review from cassidyjames and removed request for a team August 31, 2021 21:20
@andirsun
Copy link
Contributor Author

4 Months to review a PR. there is something wrong with it ?
Does not follow the roadmap of the team ?
IMHO these kind of things are not motivating to contributors like me.

Copy link
Collaborator

@jeremypw jeremypw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial code review

update_time = upower_device.update_time;

is_charging = state == State.FULLY_CHARGED || state == State.CHARGING;
is_a_battery = device_type != Type.UNKNOWN && device_type != Type.LINE_POWER;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lines 188 - 215 can be replaced by get {} clauses on corresponding read-only properties.
e.g.

public bool has_history {
    get {
        return upower_device != null ? upower_device.has_history : false;
    }
}

Comment on lines +127 to +153
public double percentage { get; private set; }
public bool is_charging { get; private set; }
public bool is_a_battery { get; private set; }
public bool has_history { get; private set; }
public bool has_statistics { get; private set; }
public bool is_rechargeable { get; private set; }
public bool online { get; private set; }
public bool power_supply { get; private set; }
public double capacity { get; private set; }
public double energy { get; private set; }
public double energy_empty { get; private set; }
public double energy_full { get; private set; }
public double energy_full_design { get; private set; }
public double energy_rate { get; private set; }
public double luminosity { get; private set; }
public double temperature { get; private set; }
public double voltage { get; private set; }
public int64 time_to_empty { get; private set; }
public int64 time_to_full { get; private set; }
public string model { get; private set; }
public string native_path { get; private set; }
public string serial { get; private set; }
public string vendor { get; private set; }
public uint64 update_time { get; private set; }
public Type device_type { get; private set; }
public Technology technology { get; private set; }
public State state { get; private set; }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use read-only properties with appropriate get {} clauses - see example below.

@danirabbit
Copy link
Member

danirabbit commented Oct 21, 2021

@andirsun andirsun closed this Jul 26, 2022
@andirsun andirsun deleted the side-bar-view branch July 26, 2022 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add battery stats/health section ComboBoxes extend to full width of window
9 participants