Skip to content

feat: Check if Include folders/files do exists (in case they are removed) #1718

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

Merged
merged 65 commits into from
May 2, 2025

Conversation

rafaelhdr
Copy link
Contributor

Solution b) when start taking a snapshot the include list should be checked of existence first and warn about missings.

reference: #1586

Solution b) when start taking a snapshot the include list should be checked of existence first and warn about missings.

reference: bit-team#1586
@rafaelhdr
Copy link
Contributor Author

rafaelhdr commented May 9, 2024

This is not complete yet, but if possible I would like some feedback before finishing it.

Are the messages good? And I am wondering if the qt warning is ok (it is asking a confirmation before continuing instead of just warning).

And I didn't implement a) for no big reason. I am still getting familiar with the code and b) was something that was more clear for me. I could try a) in a future PR.

And how you normally do translations? I was planning to translate all other languages using some automated tool (Copilot) but I wanted to confirm how you guys do it normally...

@buhtz
Copy link
Member

buhtz commented May 9, 2024

Dear Rafael,
Awesome! Thank you for this next contribution.

I added some comments and suggestions to the code. I am not sure about the whole solution. I would suggest to further discuss it in the related issue #1586.

I will set the PR into Draft mode to state the the solution is not finished.

Best,
Christian

rafaelhdr and others added 4 commits May 9, 2024 22:19
Co-authored-by: buhtz <c.buhtz@posteo.jp>
Co-authored-by: buhtz <c.buhtz@posteo.jp>
Co-authored-by: buhtz <c.buhtz@posteo.jp>
Co-authored-by: buhtz <c.buhtz@posteo.jp>
@rafaelhdr
Copy link
Contributor Author

I appreciate a lot for the review. It clarified a lot of questions and taught me some good lessons (I would never have thought about RTL issues on translations). And I am happy about not use the camel case :)

I will do the fixes from the review and wait for any suggestion in the original issue.

Thank you very much 🙏

rafaelhdr and others added 5 commits May 9, 2024 22:42
@buhtz
Copy link
Member

buhtz commented May 10, 2024

Does anyone know how to trigger the systray-icon message-bubbles via BIT?

I tried with "Snapshot.setTakeSnapshotMessage(1, 'FOO BAR')" but without success. This message appears in the status bar of the main window.

@buhtz
Copy link
Member

buhtz commented May 12, 2024

I would never have thought about RTL issues on translations

Me neither. When I started to work on the translation task I thougt it is easy. I learned things like that from translators and the community around them while trying to attract translators to BIT.

@buhtz buhtz added Discussion decision or consensus needed PR: Waiting for review PR won't be merged until review and approval from a member of the maintenance team. labels May 13, 2024
@buhtz
Copy link
Member

buhtz commented Apr 24, 2025

Hello Rafael,

I am sorry, but I see no good end for this PR. 😭 The problem is not your code but BIT's code base.

But this does not happen when doing the check in the GUI.

I don't know how to handle this. The original Snapshots.backup() code is very complex. It seems we need to do ìn the GUI something like this:

  1. mount.Mount().mount()
  2. config.PLUGIN_MANAGER.processBegin() (this seems to invoked the user-callback with reason 7)
  3. do the include-missing-check
  4. config.PLUGIN_MANAGER.processEnds()
  5. mount.Mount().umount()

I tried this but got several other errors and problems I am not able to fix quick and easy. Messing up with this mounting stuff has some implications I am not fully aware of. I need to do a lot of refactoring and also documentation to understand what mounting code is doing and how it interferes with other aspects of BIT. There are several solution options but before this refactoring and understanding need to happen.

Because I don't know what I am doing here and the high risk to destabilize BIT I do refuse to implement the GUI part.

But on the CLI the validation works and produce warnings in the logs. That is not much but better than nothing. I vote to keep this code and leave some in-code comments about.

What do you think?

Regards,
Christian

@buhtz buhtz added the PR: Unlikely to merge PR is unlikely to be merged due to project goals, technical limitations, or other constraints. label Apr 24, 2025
@buhtz
Copy link
Member

buhtz commented Apr 27, 2025

I had some time to think about it. I would say it is fine to keep the validation in the CLI component and let produce it warnings if needed. Thats it.

Someday later we find a better way to improve the communication between GUI and CLI. Than we can come back to this topic.

@buhtz buhtz added Discussion decision or consensus needed PR: Modifications requested Maintenance team requested modifications and waiting for their implementation and removed PR: Unlikely to merge PR is unlikely to be merged due to project goals, technical limitations, or other constraints. labels Apr 27, 2025
@rafaelhdr
Copy link
Contributor Author

Hello,

I agree with your concerns. I am not against holding even more this PR.

But what if we decline this PR? It is a bit frustrating but sometimes it is necessary. We could wait until we have a better communication between CLI <-> GUI, and do this feature from scratch should be quick for someone with experience on BIT. It might be faster than keep rebasing just to keep this PR alive.

@buhtz
Copy link
Member

buhtz commented Apr 28, 2025

Hello Rafael,
thanks for your reply.

I am not against holding even more this PR.

But I am. 🤣 I want bring it to an end somehow.

But what if we decline this PR?
..
sometimes it is necessary

Not an option. There is to much good work and benefit in it. ❤️
Your PR is definitely not in the "decline" category.

We could wait until we have a better communication between CLI <-> GUI

Could be years. Waiting with an open PR is not an option.

The major part of this PR is in the CLI component. I'll keep it. I'll just reduce (or somehow "decline") the GUI parts which are just the "sugar" on this PR.

If it is OK for you I'll do the modifications.

Regards,
Christian

@rafaelhdr
Copy link
Contributor Author

Sounds good :)

If it is OK for you I'll do the modifications.

Totally. I really appreciate everything you are doing 🙏

@buhtz
Copy link
Member

buhtz commented Apr 28, 2025

I really appreciate everything you are doing 🙏

And I appreciate your contribution.

Copy link
Member

@buhtz buhtz left a comment

Choose a reason for hiding this comment

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

I think that's it.

@buhtz buhtz added PR: Waiting for review PR won't be merged until review and approval from a member of the maintenance team. PR: Merge after creative-break Merge after creative-break (min. 1 week) and removed PR: Modifications requested Maintenance team requested modifications and waiting for their implementation labels Apr 30, 2025
@buhtz buhtz marked this pull request as ready for review April 30, 2025 20:27
@buhtz buhtz merged commit a953382 into bit-team:dev May 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion decision or consensus needed High PR: Merge after creative-break Merge after creative-break (min. 1 week) PR: Waiting for review PR won't be merged until review and approval from a member of the maintenance team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants