-
Notifications
You must be signed in to change notification settings - Fork 66
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
liana-gui: Iced v13 #1550
Conversation
6a1f240
to
257f5e5
Compare
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
902a373
to
0514b44
Compare
I've done some initial testing of 0514b44 and found the following: |
@@ -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> { |
There was a problem hiding this comment.
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
9a8f3ed
to
209557a
Compare
Testing 209557a:
Both fixed |
Also on 209557a: |
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"] } |
There was a problem hiding this comment.
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?
liana-gui/src/download.rs
Outdated
let response = reqwest::get(&url).await?; | ||
let total = response | ||
.content_length() | ||
.ok_or(DownloadError::NoContentLength)?; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
209557a
to
8c800a2
Compare
@@ -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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
On 8c800a2:
Fixed.
Fixed (non-selected items are no longer greyed out and background changes to green when option is highlighted). |
6e82c4c
to
178f818
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK 178f818.
liana-ui
liana-gui