Skip to content

Conversation

@Riksu9000
Copy link
Contributor

@Riksu9000 Riksu9000 commented Aug 1, 2021

I've made improvements upon @Raupinger s #342.
The notification preview can now be closed by swiping up. Maybe the notification should pop up from the bottom and be closed by swiping down?
By creating two call notifications at the same time with GB debug, the second call might stop the ringing. Can this actually happen in reality?

@Avamander
Copy link
Collaborator

Can this actually happen in reality?

Yes, when you get a VoIP call at the same time as a real call. Maybe also with call waiting.

@lman0
Copy link

lman0 commented Aug 1, 2021

if anything , i wish that your pull will be merged AFTER #342 and NOT INSTEAD by @JF002 because it will give a very bad vibe to young dev / student in cpp .

Because they would not be able to prove their ability in opensource project and they will be very reluctant to give pull if the pull is retreved/renamed by other (when for example if they are in exam period).
Then once they come back they will not try to give pull again because such pull allow them to improve their level and give them some successful impact on project as line in their resume.(a proper win/win )

Otherwise why did not you have proposed as review the easiest fix ?

i had to say it because with @JF002 doing nearly the same with another pull (where the person didn't say anything at least officially in their pull even if it's a very old pull), and you doing it on a pull that should be merged ,

i felt the project was going in a very bad path , if it were to become accepted here as normal , to take other pull and fix it (and close the original pull) instead of helping it grow by review and merging it.

@Riksu9000
Copy link
Contributor Author

if anything , i wish that your pull will be merged AFTER #342 and NOT INSTEAD by @JF002 because it will give a very bad vibe to young dev / student in cpp .

Because they would not be able to prove their ability in opensource project and they will be very reluctant to give pull if the pull is retreved/renamed by other (when for example if they are in exam period).
Then once they come back they will not try to give pull again because such pull allow them to improve their level and give them some successful impact on project as line in their resume.(a proper win/win )

Otherwise why did not you have proposed as review the easiest fix ?

I based this PR on his, so all of his commits and work exist here and with his name. I did review his PR but it has been a long time since he worked on that PR.

@lman0
Copy link

lman0 commented Aug 1, 2021

he was technically in exam period when @JF002 said he was to put it in 1.3 and even jf002 said him in discord to finish his exam before fix the pull , that jf002 can wait that the pull to be merged.

so it understandable that he was not working on it.

after some cool down , @Raupinger should be able to fix the pull .
but what i'am afraid is that he will feel useless to do it since you have done your pull and stop all improvement on pinetime if people fix on other pull instead of review.

It would this kind of situation that would make other people reluctant to give pull afterward.
("what's the point if I were to make a pull to the project but i can't improve because instead of review me , some fix it in their own way and make it merge in their own name")

@Avamander
Copy link
Collaborator

Avamander commented Aug 1, 2021

This is nothing more than a PR on top of a PR, that is quite literally how Git (and FOSS actually) works and it's how it should work. There are no authorship issues and both PRs can be closed at the same time.

There's no functional difference between having a PR merged and then making a new PR that changes the files or creating a single PR that contains commits from other PR.

@lman0
Copy link

lman0 commented Aug 1, 2021

true @Avamander that functional difference is none ,

but a student/young dev need to prove that his code is not garbage unfit to be merged and gain/have some self esteem in his/her ability.

if his hard work , is rejected/not considered but other people take his commit (even if in comment they give the proper name)

then when student/young dev show their Github account /commit to other people (for example a recruiter) these will say/show that he/she never managed to make code on big functionality that was merged and thus that they worth very few if not nothing....

because github only remember the link to pull not the comment name when in the pull of other when there is a merge.

@Avamander
Copy link
Collaborator

because github only remember the link to pull not the comment name when in the pull of other when there is a merge.

Incorrect. Rest of the text that was built on that assumption is also incorrect.

@lman0
Copy link

lman0 commented Aug 1, 2021

because github only remember the link to pull not the comment name when in the pull of other when there is a merge.

Incorrect. Rest of the text that was built on that assumption is also incorrect.

the assumption may be bad @Avamander but since
i have seen , sadly , recruiter only check sucessful merge on other opensource project , shown by github , not pull related to their not merged pull
the previous line stay true, in my experience

i will stop , talking about it because originally i wanted to only voice a warning of a situation that worrying me .

now the talk can return on the pull content itself.

@JF002
Copy link
Collaborator

JF002 commented Aug 1, 2021

Thanks @Riksu9000 for helping on that topic! Indeed, @Raupinger told us he was doing his exams and could not work on the PR right now. That's why I didn't merge it in 1.3 and move the PR in 1.4, hoping he would be able to finish the PR for the release (which is not planned yet).

I think @Riksu9000 is doing a great work by extending @Raupinger works (while keeping his commits) instead of just creating a new branch/PR.

If we decide to merge this branch, @Raupinger work will be merged in the project. See the commit history of this branch:
image
I'll just make sure to merge/rebase and not squash those commits so that they will be part of the history.

Now, for unknown reasons, sometimes Github does not automatically mark PRs as merged when they are merged by another PR or manually using CLI. If this happen for this PR, and if an eventual recruiter only look at the status of the PR instead of looking at the commit history, the "recruiter" will think @Raupinger did not succeed in adding his code in the project. But... would I take such recruiter seriously? I don't think so! And unfortunately, Github is not open source so we cannot even try to fix that issue.
I think this PR should be as valuable, and maybe more valuable than any other PR : @Raupinger started some work, @Riksu9000 extended it, which shows that the code was good enough to allow multiple people to work on the same topic, and any competent recruiter should be able to see that.

i will stop , talking about it because originally i wanted to only voice a warning of a situation that worrying me .

I heard your voice and I'll make sure the work of all contributors will be visible in the commit history following the state of the art of Git ;)

@lman0
Copy link

lman0 commented Aug 1, 2021

I'll just make sure to merge/rebase and not squash those commits so that they will be part of the history.

thanks @JF002 , please do so for other pulls as well .

the other warning/(voicing?) was that it's good to improving another pull as long as :
the original commit are properly put in history (no squash that make the original/previous pr commit disappear) ,
and
he original pull did received some review in order that the dev could grow thank to the reviews .

Otherwise young dev will hardly improve themselves.("the forging tales effect")

by the way ,
since @Riksu9000 didn't know about @Raupinger situation((because it was only written on discord)
it understandable that she/he have created a pull (it was month(s) already of no working on it shown on github)
and since @Riksu9000 properly put the commit in his name (instead of squashing , like i've seen on other project)
@Raupinger will still be "rewarded" ,by working on his pull
And @Riksu9000 will be "rewarded" by fix it and improving it and maybe add personal functionality on her own pull

@Raupinger
Copy link
Contributor

Raupinger commented Aug 1, 2021

Huh, that's quite the wall of text you're treating me here. Since there was some talk about my situation let my weigh in.

I don't do any of this because I'm trying to build a resume. I decided to contribute to Infinitime because I enjoy building things and I've wanted to take part in open source for quite a while. This is the first project where it seemed like I could make an impact (KDEs stuff confused the heck out of me for example). If you take a look at my GitHub you'll find plenty of unfinished projects that are the result of a young person biting of way more than they could chew. I've decided to leave all of that public because for me, this isn't about cosmetics, its about me enjoying to learn new things. (And I still learned a lot from those failed projects).

Even with all that said, I still have the GitHub link in my CV and will point out things like Infinitime, but there are already some PRs authored by me that have been merged and I intend to continue working on the ones that are still open as well as new ones. a single PR more or less won't make a difference to a sane recruiter.

My exams are over btw. and I'm exited to get back to writing code in the next few days.

PS.: it's not @JF002's job as a maintainer to help me or other people look good on their CVs

@Raupinger
Copy link
Contributor

It would probably still be smart for me to review those new changes so we avoid bugs from @Riksu9000 misinterpreting the code I wrote.

Copy link
Collaborator

@Avamander Avamander left a comment

Choose a reason for hiding this comment

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

Looks nice formatting-wise.

Have you taken a look at clang-tidy warnings, didn't create any new ones?

@Riksu9000
Copy link
Contributor Author

I assume clangd shows all clang-tidy warnings, so I don't think I created new ones.

@Riksu9000
Copy link
Contributor Author

Yes, when you get a VoIP call at the same time as a real call. Maybe also with call waiting.

I just realized there might be another separate issue here as well. If there are two call notifications at the same time, and we press end call, does the correct call get ended? It doesn't look like the notification InfiniTime sends contains information about which call was ended, so what will happen?

@Riksu9000 Riksu9000 mentioned this pull request Aug 6, 2021
@JF002 JF002 merged commit 0eeed5a into InfiniTimeOrg:develop Aug 14, 2021
@JF002 JF002 added this to the Version 1.4 milestone Aug 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants