paper-plane icon indicating copy to clipboard operation
paper-plane copied to clipboard

Initial document message

Open yuraiz opened this issue 2 years ago • 8 comments

I made the support for document message

downloading

Todo:

  • [X] Remake widget ui
  • [x] Add custom css style
  • [X] Add action to cancel loading
Make in other requests
  • [ ] Add loading indicator
  • [ ] Add action to remove the downloaded document
  • [ ] Add thumbnails
  • [ ] (Idea) add drag and drop to move as file

I'm new to programming using gtk so any suggestions are welcome

yuraiz avatar Aug 29 '22 18:08 yuraiz

Thanks a lot for this PR!

I wrote an initial review about the general widget structure. If you need any help feel free to ask!

Some quick comments about the TODO tasks:

Remake widget ui (using boxes?)

Yeah, I left a review for how this could be managed using GtkBoxes.

Make property to change loading status

I don't think it's really useful having a property for that, you can probably just react to changes in the receiver callback and update the widget.

Add spinner to indicate loading

I'm not sure if you mean an actual progress animation or just a GtkSpinner. For the former we'd need a custom widget to achieve the same animation as seen in other Telegram clients (radial progress bar). For the latter, instead, I think it's not a great option because it can't show the progress, so we could just show the cancel button to indicate that it's downloading.

Add action to remove the downloaded document

I don't think this is really useful, I don't see other clients doing something like this.

Add thumbnails (low priority)

I think you can skip this for this PR so that we can concentrate about the basic widget first.

melix99 avatar Aug 29 '22 20:08 melix99

I updated ui but now i can't open some files

image

upd: now for some reason i can

yuraiz avatar Aug 30 '22 10:08 yuraiz

Maybe add different colors for different file types like Telegram X, but i don't found this in standart clients

The problem with this is that it's really difficult to recolor the app (we'll support the official themes as seen in the official clients) and still assure a good contrast for every file type color.

About CSS style I suggest something like this:

messagedocument > box > button {
  background: @accent_color;
  color: @window_bg_color;
}

messagedocument.outgoing > box > button {
  background: @accent_fg_color;
  color: @accent_bg_color;
}

This will result in this result, which assures a good contrast (ignore the low contrast sender color, that's a different bug :p):

image

melix99 avatar Aug 30 '22 14:08 melix99

Now i can cancel downlading

cancel action

Other changes:

  • I've changed some logic in code for ease of handling different statuses
  • Now the status_button doesn't accumulate handlers (I thought that Button::connect_clicked automatically deletes the old action)
  • Now loading status hides size of file (i think without automatic downloading it's useless)
  • Style is updated

yuraiz avatar Aug 31 '22 14:08 yuraiz

Playing with thumbnails, but i don't know good way to fit image into square

playing-with-thumbnails.webm

here's the bad way:

let pixbuf =
    gdk::gdk_pixbuf::Pixbuf::from_file(&thumbnail.file.local.path).unwrap();

let new_pixbuf = gdk::gdk_pixbuf::Pixbuf::new(
    gdk::gdk_pixbuf::Colorspace::Rgb,
    false,
    pixbuf.bits_per_sample(),
    320,
    320,
)
.unwrap();

let scale = thumbnail.width as f64 / thumbnail.height as f64;

let offset_x = -(thumbnail.width as f64 - thumbnail.height as f64);

pixbuf.composite(
    &new_pixbuf,
    0,
    0,
    320,
    320,
    offset_x,
    0.0,
    scale,
    scale,
    gdk::gdk_pixbuf::InterpType::Nearest,
    255,
);

let pixbuf = new_pixbuf;

let picture = gtk::Picture::for_pixbuf(&pixbuf);

yuraiz avatar Aug 31 '22 16:08 yuraiz

Playing with thumbnails, but i don't know good way to fit image into square

We probably want to wait for GTK 4.8 where I've added a property to GtkPicture to control just that. For what I've heard a new version of gtk4-rs is coming soon with the new APIs, so we'll switch to 4.8 when that release will happen.

melix99 avatar Sep 01 '22 15:09 melix99

Almost there. I see some remaining issues regarding the commits:

Commit Message

The first word after the tag should be capitalized, and you should use imperative style message-document: added clickable area -> message-document: Add clickable area

Split of the Commits

I see some issues how you split up the commits. For example, you fix style issues in the second commit that you introduced in the first commit. That produces a lot of noise because the information that you fixed things within the PR isn't relevant after it has been merged. Besides, it distracts the reviewer.

A better split would be to introduce the necessary types (document) in the first commit and wire things up in the second commit (show document messages in the chat). But I think, in this case, it's also fine to squash everything into one commit.

marhkb avatar Sep 11 '22 20:09 marhkb

Very Good. You have my approval if you add an imperative verb to the summary line of the commit message. For example message-document: Implement initial version or message-document: Implement basic functionality

For commit guidelines you can refer to https://wiki.gnome.org/Git/CommitMessages

marhkb avatar Sep 12 '22 07:09 marhkb

Last thing, do you mind squashing everything into a single commit?

melix99 avatar Oct 09 '22 13:10 melix99

Thanks!

melix99 avatar Oct 09 '22 13:10 melix99