Skip to content
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

Feat: script editor in pre and post backup scripts #1950

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

SAMAD101
Copy link
Collaborator

@SAMAD101 SAMAD101 commented Feb 28, 2024

Description

I've added buttons beside the pre-backup and post-backup line edits in the Schedule tab in the Shell Commands section.

untitled.mp4

image

Related Issue

#1945

Motivation and Context

Makes it more convenient to add the commands using a shell script.

How Has This Been Tested?

does effect much of the code, only extends some functionality

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have read the CONTRIBUTING guide.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

I provide my contribution under the terms of the license of this repository and I affirm the Developer Certificate of Origin.

@shivansh02
Copy link
Collaborator

Oh even I was ready with a PR, I shared a screenshot in the issue thread. Could've saved one of us the work if you told me that you were working on this 😅

@SAMAD101
Copy link
Collaborator Author

Oh even I was ready with a PR, I shared a screenshot in the issue thread. Could've saved one of us the work if you told me that you were working on this 😅

damn 😔 yeah I should've. I just finished this one today.

@m3nu
Copy link
Contributor

m3nu commented Feb 29, 2024

To avoid overlaps, you can make sure the issue is assigned to you before working on it.

As for this implementation, it's reading a file. We should also look into having a larger editor window instead. Often it's hard to decide what's the best way without seeing it. So even PRs that aren't merged can help with discovering a good solution.

@SAMAD101
Copy link
Collaborator Author

To avoid overlaps, you can make sure the issue is assigned to you before working on it.

As for this implementation, it's reading a file. We should also look into having a larger editor window instead. Often it's hard to decide what's the best way without seeing it. So even PRs that aren't merged can help with discovering a good solution.

Thank you
Will try implementing that as well.

PS: please assign me the issue #1945

@SAMAD101
Copy link
Collaborator Author

SAMAD101 commented Mar 1, 2024

Well, apparently I have exams starting soon and also have to ready my proposal for GSOC. So, I'll add this one along with some more issues in my proposal.

@SAMAD101
Copy link
Collaborator Author

SAMAD101 commented Mar 2, 2024

To avoid overlaps, you can make sure the issue is assigned to you before working on it.

As for this implementation, it's reading a file. We should also look into having a larger editor window instead. Often it's hard to decide what's the best way without seeing it. So even PRs that aren't merged can help with discovering a good solution.

Well, I snatched some time and implemented a small editor window for it. I've updated this PR. This is how it looks so far:
Screenshot_20240303_012208
Screenshot_20240303_014727

PS: monospace fonts aren't installed in my system, so that must be why it is appearing in simple noto font.

@m3nu
Copy link
Contributor

m3nu commented Mar 4, 2024

Editor looks good. Just what I imagined.

I'd remove the "Load from file" button though. Since the problem it solves is very minor. One can easily copy and paste in the editor. Would just make sure the editor window can be resized.

@SAMAD101
Copy link
Collaborator Author

SAMAD101 commented Mar 4, 2024

Editor looks good. Just what I imagined.

I'd remove the "Load from file" button though. Since the problem it solves is very minor. One can easily copy and paste in the editor. Would just make sure the editor window can be resized.

sure, that can be done. Yeah, The editor window is resizable, will make it wider ig.

@SAMAD101
Copy link
Collaborator Author

SAMAD101 commented Mar 4, 2024

Update:
Screenshot_20240304_135711
Screenshot_20240304_135717

@SAMAD101 SAMAD101 changed the title Feat: script addition in pre and post backup scripts using file dialog. Feat: script editotr in pre and post backup scripts using file dialog. Mar 4, 2024
@SAMAD101 SAMAD101 changed the title Feat: script editotr in pre and post backup scripts using file dialog. Feat: script editor in pre and post backup scripts using file dialog. Mar 4, 2024
@SAMAD101 SAMAD101 changed the title Feat: script editor in pre and post backup scripts using file dialog. Feat: script editor in pre and post backup scripts Mar 4, 2024
@shivansh02
Copy link
Collaborator

some spacing between the field and Save button would look good IMO.

@m3nu
Copy link
Contributor

m3nu commented Mar 4, 2024

Looks good to me. Will try it locally and then merge, if there are no other concerns or things that come up.

@SAMAD101
Copy link
Collaborator Author

SAMAD101 commented Mar 4, 2024

Updated

Before Screenshot_20240304_135711
After Screenshot_20240304_152540

Copy link
Contributor

@diivi diivi left a comment

Choose a reason for hiding this comment

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

IMO the design looks a bit off, attaching some issues:

Too much space here

I did something similar with the Exclude GUI last year, the UI for that looked decent and referring to that might be good for consistency. Here's a screenshot:

image

(Ignore the tabbed button above) Nice work otherwise!

Also, the title ("Edit Pre-Backup Script") for the dialogs is different but the header text ("Custom Pre/Post backup script") is still the same, maybe you can change that too?

@SAMAD101
Copy link
Collaborator Author

SAMAD101 commented Mar 5, 2024

Updated:

  • Now the label does change depending upon the context (pre/post-backup script).
  • Controlled the spacing. Looks more aligned now.
  • Use of QDialogButtonBox for 'Save' and 'Cancel' buttons.

Screenshot_20240305_134224

@SAMAD101
Copy link
Collaborator Author

SAMAD101 commented Mar 5, 2024

Now that it's complete, how about I add some syntax highlighting to it as well? Need some thoughts.

@real-yfprojects
Copy link
Collaborator

Now that it's complete, how about I add some syntax highlighting to it as well? Need some thoughts.

Seems to much efford for such a small use case. Most people will copy paste the script from another editor anyways.

@SAMAD101
Copy link
Collaborator Author

SAMAD101 commented Apr 1, 2024

Seems to much efford for such a small use case. Most people will copy paste the script from another editor anyways.

Yeah, I'll drop the idea. Its fine as it is.

@SAMAD101 SAMAD101 requested a review from m3nu May 3, 2024 22:31
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