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

liana-gui: Iced v13 #1550

Merged
merged 8 commits into from
Feb 4, 2025
Merged

Conversation

edouardparis
Copy link
Member

@edouardparis edouardparis commented Jan 21, 2025

  • update liana-ui
  • update liana-gui

edouardparis added a commit that referenced this pull request Jan 28, 2025
3ced1db move the unlocking of bb to a method (edouardparis)

Pull request description:

  This is preparatory work for #1550

ACKs for top commit:
  edouardparis:
    Self-ACK 3ced1db

Tree-SHA512: 1ff5aef26ed13446750a5f1c339d47b35a15227df98bb57321f81520294f35fd23fd226f3ccf515a56f07b8ec5b4f1d9c0fcb854f743d862389ab4605afcd3df
@edouardparis edouardparis force-pushed the iced-v13 branch 8 times, most recently from 902a373 to 0514b44 Compare January 31, 2025 15:31
@jp1ac4
Copy link
Collaborator

jp1ac4 commented Jan 31, 2025

I've done some initial testing of 0514b44 and found the following:

  1. Items in dropdowns no longer have green borders:
    image

  2. Error message banners are no longer displaying correctly:
    image
    image

  3. Signing device rows in the modal are no longer clearly marked:
    image

@@ -11,98 +11,98 @@ pub const P1_SIZE: u16 = 16;
pub const P2_SIZE: u16 = 14;
pub const CAPTION_SIZE: u16 = 12;

pub fn h1<'a>(content: impl Into<Cow<'a, str>>) -> iced::widget::Text<'a, Theme> {
iced::widget::Text::new(content)
pub fn h1<'a>(content: impl Display) -> iced::widget::Text<'a, Theme> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe here it worth it to have a macro to build all these helper functions as they all have the same signature? it could be usefull for readability & further maintenance

@edouardparis edouardparis force-pushed the iced-v13 branch 2 times, most recently from 9a8f3ed to 209557a Compare February 3, 2025 11:13
@jp1ac4
Copy link
Collaborator

jp1ac4 commented Feb 3, 2025

Testing 209557a:

2. Error message banners are no longer displaying correctly:
3. Signing device rows in the modal are no longer clearly marked:

Both fixed
..
Some new issues found:

  1. The address shown below the QR code was previously fitting on a single line:
    image

  2. "Delete wallet" button background does not change colour when hovered:
    image

@jp1ac4
Copy link
Collaborator

jp1ac4 commented Feb 3, 2025

Also on 209557a:

  1. The "0 payment" text seems to be a different colour/boldness than the other rows:
    image

@pythcoiner
Copy link
Collaborator

pythcoiner commented Feb 3, 2025

  1. The address shown below the QR code was previously fitting on a single line:

previoulsy the address were not 100% displayed w/ taproot, there is an issue for this

see #1051

@jp1ac4
Copy link
Collaborator

jp1ac4 commented Feb 3, 2025

previoulsy the address were not 100% displayed w/ taproot, there is an issue for this

see #1051

Ah I see. Note that the address in my screenshot above is displayed on a single line in master, but we can come back to this then as part of your PR.

@pythcoiner
Copy link
Collaborator

previoulsy the address were not 100% displayed w/ taproot, there is an issue for this
see #1051

Ah I see. Note that the address in my screenshot above is displayed on a single line in master, but we can come back to this then as part of your PR.

yeah i think there is a new feature that wrap the line if too long content

@@ -48,7 +48,7 @@ chrono = "0.4.38"
# Used for managing internal bitcoind
base64 = "0.21"
bitcoin_hashes = "0.12"
reqwest = { version = "0.11", default-features=false, features = ["json", "rustls-tls"] }
reqwest = { version = "0.11", default-features=false, features = ["json", "rustls-tls", "stream"] }
Copy link
Collaborator

@pythcoiner pythcoiner Feb 3, 2025

Choose a reason for hiding this comment

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

maybe the PR summary should indicate what the stream feature is used for?

let response = reqwest::get(&url).await?;
let total = response
.content_length()
.ok_or(DownloadError::NoContentLength)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should not error in this case, it's better to get the file w/o update of the download process instead not get the file at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

We expect it still to be here as we download a static file. I think I should not modify the logic from the iced example.

Copy link
Collaborator

Choose a reason for hiding this comment

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

from reqwest:

    /// Get the content-length of this response, if known.
    ///
    /// Reasons it may not be known:
    ///
    /// - The server didn't send a `content-length` header.
    /// - The response is compressed and automatically decoded (thus changing
    ///   the actual decoded length).
    pub fn content_length(&self) -> Option<u64> {

I think we should not error in these mentionned cases

Copy link
Member Author

Choose a reason for hiding this comment

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

178f818
Ok, in that case, we do not send the progress Message as we are not able to calculate it.

@edouardparis edouardparis marked this pull request as ready for review February 3, 2025 15:42
@@ -377,6 +327,65 @@ impl Loader {
}
}

fn get_bitcoind_log(log_path: PathBuf) -> impl Stream<Item = Option<String>> {
channel(1, move |mut output| async move {
loop {
Copy link
Collaborator

Choose a reason for hiding this comment

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

is that intended that we never break this loop?

Copy link
Member Author

Choose a reason for hiding this comment

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

From my understanding, the iced runtime will kill the stream as soon as the subscription is not returned.
https://docs.rs/iced/latest/iced/struct.Subscription.html#the-lifetime-of-a-subscription

@jp1ac4
Copy link
Collaborator

jp1ac4 commented Feb 4, 2025

On 8c800a2:

2. "Delete wallet" button background does not change colour when hovered:
3. The "0 payment" text seems to be a different colour/boldness than the other rows:

Fixed.

  1. Items in dropdowns no longer have green borders:

Fixed (non-selected items are no longer greyed out and background changes to green when option is highlighted).
As noted elsewhere, the dropdown no longer has a green border when hovered, but this is an intended change.

Copy link
Collaborator

@jp1ac4 jp1ac4 left a comment

Choose a reason for hiding this comment

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

tACK 178f818.

@edouardparis edouardparis merged commit 3a4df7a into wizardsardine:master Feb 4, 2025
11 of 12 checks passed
@edouardparis edouardparis deleted the iced-v13 branch February 4, 2025 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants