Action Bugfixes & Support for Action Icons

[ValentijnvdBeek]

I removed the translation changes and cleaned up one of the commits (I genuinely don't know why git decided to add the translation changes to it).

Hopefully they are clean enough now, however it did pull together the commit fixing the bug where actions wouldn't update and the commit where I added the action-icon action. Hopefully these are close enough together that I don't have to pull them further apart because wrestling with git hasn't been fun..

However if you do want them or anything else separate (or any other chances) I would love to hear it of course. I could also separate them as separate pull requests but that still strikes me as a bit weird.

I closed the last PR for the clean commit history. Feel free to keep yelling at me.

EDIT: Also take your time and enjoy your vacation.

[luis-pereira]

@ValentijnvdBeek As you can see this PR has merge conflicts. That's not good for merging.
I solved all the merge conflicts and removed the commits that already landed on master. The result is here https://github.com/luis-pereira/lxqt-notificationd/tree/actions. It's much amenable to work with. If those are your intended changes, push (-f) onto this PR and then we will start the review process.

p.s. At any time did I well with you, or anybody else.
p.s2. I'm still in a flaky connection situation.

[ValentijnvdBeek]

@luis-pereira It shouldn't have any merge conflicts (or at least Github says it has none) but honestly I am not terribly surprised since Git really dislikes it when somebody mucks around with it's history.

I don't think we'll ever agree on this point but I still think you made some mistakes that it would suit you to recognize and fix as I do with my own. If only to appeal more to new contributors because I would have loved to do some more work on this project but I really don't want to work with people who tell me that I have unmet emotional needs that I am forcing on people. Surprisingly enough I am also a person with a limited amount of hours in a day.

[agaida]

It can't be rebased - it should be in a state that can be rebased - so, where is the problem?

[ValentijnvdBeek]

@agaida I just did exactly what he asked me to do without complaining? I just said that Github said it could be merged but that I wouldn't be surprised if it is wrong since Git really dislikes it when you change the history in any way. I was saying he was right.

[luis-pereira]

@ValentijnvdBeek After 6ff94e1 it hasn't merge conflicts.

I don't think we'll ever agree on this point but I still think you made some mistakes that it would suit you to recognize and fix as I do with my own.

I really can't understand the meaning of it. I don't know what you are talking about. You don't explain.

Exactly when did I say that you "have unmet emotional needs that I am forcing on people" ?

[ValentijnvdBeek]

@luis-pereira It said before, on Github, that "This branch had no conflict with the base branch" but that doesn't really matter anymore since I already changed it. I still have the original code so I could prove it but I really don't think that every single should have a massive fight associated with it.

"Yes, of course. But you seem to have emotional demands. Here is not the place for them.".
Which, to be fair, wasn't said by you but I am not holding you accountable for those words. I however do feel justified in holding it against him and the organization.

[luis-pereira]

@ValentijnvdBeek We agree that I didn't say that. So, why bring it up here ?
Let's focus on the technical details.
The PR is now in an state where it can be reviewed and tested.

The first thing that comes to mind is: How do I test this ?
It would be very useful to have a test script inlxqt-notificationd/test

You changed the commend style (and apparently only that), why ? In any case style changes should be placed in it's own commit. https://github.com/lxqt/lxqt/wiki/Contributing-code#clean-commits

... to be continued, real life calls

[ValentijnvdBeek]

@luis-pereira Because somebody who is a member of LXQt who speaks on an official LXQt channel with the label member next to his name speaks for all of you. You endorse or at least condone this type of behaviour and I think that is wrong. But honestly can we let this go now.

The problem with writing a test is that the current way that lxqt-notificationd is being tested is inadequate to test these changes. notify-send doesn't support actions which means that the part of lxqt-notificationd that I changed can't be tested using it. This has as an immediate result that the action functionality is broken since it wouldn't update the actions at all, but would either not do anything or drop the old actions and just append the new ones. Luckily this does get fixed in this commit but it does indicate that the way that lxqt-notificationd gets tested should be retaught and the tests rewritten. Anything less would be mopping the floor while the faucet is still flowing.

I uncommented those lines for testing reasons and recommented them when I was finished. I didn't end up doing them at exactly the same place but since it is very small change and didn't impact the code at all I ended up leaving it be. Also the thing you linked doesn't actually say that but instead says:

The commits themselves should be clean! Each commit should be working independently, add one feature/fix per commit and the commit messages should be descriptive of the change and properly spelled/capitalized. This is important for bisecting bugs and reading the log pleasantly.

Style changes are neither fixes or features so they don't fall under this rule but that is minor nitpick. My bigger crime in this department is that my commit adds a feature and fixes a bug (which was just two lines) which I can fix if you want.

The same document also states:

In most cases, the code should have no debug statements left over from testing and no commented out functionality.

Which I think implies that those lines that were changed shouldn't be there to begin with since they are commented debug statements. So if you want me to change them, I should probably go all the way and delete them outright.

[tsujan]

You endorse or at least condone this type of behaviour

Again!

(1) Hoping to stop wasting of time (which has become very irrelevant now), in a comment, I said what was correctly quoted above. Even if one equates "demand" with "need" and freely adds the word "unmet" to it, there's nothing in that comment telling that "emotional needs" are wrong or denying that all people on earth may have "unmet emotional needs" -- whatever that phrase may mean or however it's coined. The emphasis was on the place.

(2) Rationally, if I'm a reproachable person, that says nothing about an organization I work with, a toolkit I use, a piece of code I write, etc. Irrationally, everything is related to everything.

(3) Apparently, @ValentijnvdBeek can continue this ad infinitum. The only one who can stop him is himself.

[ValentijnvdBeek]

@tsujan I wasn't the person who started this conversation up again and I was hoping that we could avoid it since we are never going to agree. Now that we all had our say can we go back to determining that I am both a terrible person AND a terrible programmer :)

[tsujan]

@ValentijnvdBeek It's your choice to interpret things that way. I can just use words and say that only codes are important here.

[luis-pereira]

@luis-pereira Because somebody who is a member of LXQt who speaks on an official LXQt channel with the label member next to his name speaks for all of you. You endorse or at least condone this type of behaviour and I think that is wrong. But honestly can we let this go now.

I'm Portuguese and by your logic I endorse everything the elected government states !
I did let it go. You were the one that bring it up again.
We are all adults, each person speaks for itself.

The problem with writing a test is that the current way that lxqt-notificationd is being tested is inadequate to test these changes. notify-send doesn't support actions which means that the part of lxqt-notificationd that I changed can't be tested using it. This has as an immediate result that the action functionality is broken since it wouldn't update the actions at all, but would either not do anything or drop the old actions and just append the new ones. Luckily this does get fixed in this commit but it does indicate that the way that lxqt-notificationd gets tested should be retaught and the tests rewritten. Anything less would be mopping the floor while the faucet is still flowing.

lxqt-notificationd is being tested in an inadequate manner, for your new feature ? So, write a adequate test suit. I and all developers would be very glad to see it.
Nobody forces you to use send-notify

I uncommented those lines for testing reasons and recommented them when I was finished. I didn't end up doing them at exactly the same place but since it is very small change and didn't impact the code at all I ended up leaving it be.

That pollutes the history. If it does nothing, why should it be there ? It's as simple as that.
Ammending the commit is much quicker that all this talk.

BTW I just don't know where the terrible person and terrible programmer.

[agaida]

@ValentijnvdBeek: You might find it unkind, but you should work on your code/noise ratio. Nobody is pissed about some hard contributors rants if the ratio is 1/(current ratio). Right now it is a pain to read for every potential reviewer.

[ValentijnvdBeek]

@luis-pereira

lxqt-notificationd is being tested in an inadequate manner, for your new feature ?

Not exactly, but close. It wasn't able to test actions at all which my pull request touches on.

So, write a adequate test suit. I and all developers would be very glad to see it.

I just pushed a commit which ports the existing tests to Python 3 and added another test for actions.

That pollutes the history. If it does nothing, why should it be there ? It's as simple as that.
Ammending the commit is much quicker that all this talk.

You asked me why and I answered your question. I just amended the commit since I didn't have the time do so before (maths exam).

[luis-pereira]

Back from vacation. Will review ASAP.