search

cdepillabout / termonad

Terminal emulator configurable in Haskell.
https://hackage.haskell.org/package/termonad
BSD 3-Clause "New" or "Revised" License
401 stars 49 forks source link

Use embedded icon - Part II #228

Closed refaelsh closed 1 year ago

refaelsh commented 1 year ago

create a new branch, and create a new PR

I've created a new PR in continues of this: https://github.com/cdepillabout/termonad/pull/227.

refaelsh commented 1 year ago
  1. This is now ready for code review.
  2. On line 383, I don't really know the correct numbers. Do you?
refaelsh commented 1 year ago

I can check it when I review the PR.

Can you please? I am kinda lazy on that regard :-)

gelisam commented 1 year ago

On line 383, I don't really know the correct numbers. Do you?

Those numbers are really important! They indicate the format in which the color data is encoded. According to the documentation, the numbers you used are saying that the bytes you are providing are in the RGB8 format and are describing a 16x16 image.

I'd give an example, but 16x16 is a lot, so let's simplify and pretend you are describing a 2x2 image whose pixels are red at the top-left, grey at the top-right, white at the bottom-left, and white at the bottom-right. In hex, those 3 colours are:

  1. red is FF0000
  2. grey is 888888
  3. white is FFFFFF

You would thus need to provide those 12 bytes:

FF 00 00 88 88 88 FF FF FF FF FF FF

Are those the bytes you are providing? Not at all! You have loaded the bytes of a png file into memory, but a png is not stored as an array of RGB8 pixels. For one thing, it starts with magic numbers indicating that the file is a png, and then a header indicating the size of the file and some other metadata, and the compressed color data.

I would again provide an example, but the real png format is really complicated, so let's just pretend like the png format for that 2x2 image looks like this:

PNG
2 2
1 FF0000
1 888888
2 FFFFFF

That is, this is a PNG, the image has size 2x2, the first 1 pixels have color FF0000, the next 1 pixels have color 888888, the next 2 pixels have color FFFFFF.

In hex, "PNG" is 50 4E 47, so the png file consists of the following bytes:

50 4E 47 02 02 01 FF 00 00 01 88 88 88 02 FF FF FF

You are interpreting those bytes as color values, so the first 12 bytes will be interpreted as four seemingly-random colors:

  1. the top-left pixel will have color 504E47 (a dark grey)
  2. the top-right pixel will have color 020201 (basically black)
  3. the bottom-left pixel will have color FF0000 (red, we know that one)
  4. the bottom-right pixel will have the color 018888 (teal)

I recommend using the JuicyPixels library to load the png into the RGB8 format.

refaelsh commented 1 year ago

Yes, I understand. Thank you very much for the explanation. I will need some time to process it all, but I think I understand.

refaelsh commented 1 year ago

I recommend using the JuicyPixels library to load the png into the RGB8 format.

I will go and read the documentation of this right now.

cdepillabout commented 1 year ago

@gelisam Great explanation, thanks a lot for that!

refaelsh commented 1 year ago

I did another commit that is code review quality.

cdepillabout commented 1 year ago

@refaelsh Sorry it took my a while to get to this!

Here's my understanding the current status of this:

  1. We expect that the current code in this PR will work for setting the application icon.
  2. @refaelsh hasn't actually been able to test this, because the application icon doesn't appear anywhere when running Termonad in XMonad.
  3. @gelisam has tested this PR in a GTK4-based Gnome environment, and found that neither the current Termonad code (from master), nor this PR appear to set the application icon correctly. So it appears that this PR is at least not making things worse.

Is that correct? If so, I'll have to put together a GTK3-based Gnome environment and confirm that this PR correctly sets the application icon.

refaelsh commented 1 year ago

Sorry it took my a while to get to this!

That's perfectly ok! All is fine :-)

Is that correct?

Yes.

gelisam commented 1 year ago

@gelisam has tested this PR in a GTK4-based Gnome environment,

I'm using Gnome version 42.4; I have no idea whether that version is based on GTK3 or GTK4.

refaelsh commented 1 year ago

@gelisam has tested this PR in a GTK4-based Gnome environment,

I'm using Gnome version 42.4; I have no idea whether that version is based on GTK3 or GTK4.

Interesting question. Google did not help me here, not a definite answer any way. Best guess: GTK4.

cdepillabout commented 1 year ago

I took a look at the icon from this PR in the VM added in #230, and while it does appear that the icon is getting set, it looks like it is not rendering correctly:

You can see it in the title bar here:

image

And a larger image here?

image

Maybe there is some way you could easily write the final PixBuf to a file, and use that for debugging?

refaelsh commented 1 year ago

@cdepillabout Thank you for checking it in a VM!

Maybe there is some way you could easily write the final PixBuf to a file, and use that for debugging?

Good suggestion. I will do it and ping back here when its done.

refaelsh commented 1 year ago

Good suggestion. I will do it and ping back here when its done.

I did it. There is a file with an icon that is created. It looks ugly, but it exists --> I assume that all the shenanigans with converting to this Pixbuf thingy works. Again, the icon comes out distorted, but it's there. I am attaching the resulting file. bla

gelisam commented 1 year ago

The fact that it is distorted means that the numbers are incorrect. For example, maybe the image has 4 bytes per pixel and you're interpreting them as 3 bytes per pixel. So if the original image is

<red> <red>  <red>
<red> <blue> <red>
<red> <red> <red>

and each pixel is encoded as 4 bytes, so each row has 3 groups of 4 bytes:

[FF 00 00 FF] [FF 00 00 FF] [FF 00 00 FF]
[FF 00 00 FF] [00 00 FF FF] [FF 00 00 FF]
[FF 00 00 FF] [FF 00 00 FF] [FF 00 00 FF]

But you incorrectly interpret each row as 4 groups of 3 bytes:

[FF 00 00] [FF FF 00] [00 FF FF] [00 00 FF]
[FF 00 00] [FF 00 00] [FF FF FF] [00 00 FF]
[FF 00 00] [FF FF 00] [00 FF FF] [00 00 FF]

Then you end up with an image which looks kind of like the original, but with weird vertical color bands:

<red> <yellow> <turquoise> <blue>
<red> <red>    <white>     <blue>
<red> <yellow> <turquoise> <blue>
refaelsh commented 1 year ago

@gelisam Thank you very much, this helps. I will indeed continue working to make the image not distorted.

refaelsh commented 1 year ago

Short status update: I've managed to improve the distortion in the icon, it looks like this now: debug_icon The colors are a bit off, and it appears to be flipped vertically.

I will continue working on this.

refaelsh commented 1 year ago

Short update: I fixed the horizontal flip. The image looks now identical to the original. The code is not ready for review. I will continue working on it.

refaelsh commented 1 year ago

The code is ready. @cdepillabout, Can you please verify the fix via your VM method?

refaelsh commented 1 year ago

I am now noticing that my formatter made a lot of changes. I use what comes by default with GHC and/or haskell-language-server. What do you use?

cdepillabout commented 1 year ago

Can you please verify the fix via your VM method?

Sure, I can do this, but I probably won't have any time for at least the next 3 weeks. I'll get back to you again after I have time to test this out.

I am now noticing that my formatter made a lot of changes. I use what comes by default with GHC and/or haskell-language-server. What do you use?

I actually don't use a formatter on the Termonad codebase. My personal style is somewhat similar to the default style in hindent, but enough people have worked on the Termonad codebase now that different sections have slightly different formatting styles.

I may ask you to remove the formatting-specific changes in this PR (so that it is easier to just see the code you have added), but you could always just do that after I confirm in the VM.

refaelsh commented 1 year ago

Sure, I can do this

Thank you very much :-)

but I probably won't have any time for at least the next 3 weeks

Sure. No problem. What's best for you.

but you could always just do that after I confirm in the VM.

Yes. Understood and agreed.

refaelsh commented 1 year ago

@cdepillabout Can I please bother you for a minute of your time? Please go and upvote/+1/like this question: https://github.com/xmonad/xmonad/discussions/472. If there is an answer, it might save the whole VM time spin up in the future.

cdepillabout commented 1 year ago

I was able to test this, it is looking good!

Here are shots from the VM:

image

You can see the icon is looking correct!

@refaelsh Could you rebase this on master, and get rid of the formatting changes? After you do that, I'll merge this in!

refaelsh commented 1 year ago

@refaelsh Could you rebase this on master

Sure. No problem.

and get rid of the formatting changes

Off course. I am on it.

refaelsh commented 1 year ago

I did something wrong. I am not really sure what. Not only that, but I've done many rebases in my life but never on a forked repo. I will close this PR and open a new one. I don't see any other choice. Is that ok by you?

cdepillabout commented 1 year ago

@refaelsh Yeah, no problem!