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.

Sign up for GitHub

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

Jump to bottom

feat(Submit): Improve and add action menu #681

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

feat(Submit): Improve and add action menu #681

wants to merge 38 commits into from

Conversation

coder3101
Copy link
Member

@coder3101 coder3101 commented Nov 20, 2020 •
edited

This commit improves and add following:

  1. "Submit" in Action menu
  2. Shortcut for "Submit", default is Ctrl+Alt+S
  3. Improve Submit logic, and warns when submission through old cf tool.

Related Issues / Pull Requests

Fixes #679 and Fixes #201

Motivation and Context

This is minor changes to CF Tool and submission.

How Has This Been Tested?

Arch Linux with KDE

Screenshots (if appropriate)

Checklist

  • If the key of a setting is changed, the old attribute is updated or it is resolved in SettingsUpdater.
  • If there are changes of the text displayed in the UI, they are wrapped in tr() or QCoreApplication::translate().
  • If needed, I have opened a pull request or an issue to update the documentation.
  • If these changes are notable, they are documented in CHANGELOG.md.

Additional text

In future we will re-write submission logic, till then either this PR gets merged or updated to reflect the changes.

@coder3101 coder3101 added awaiting for docs Associated PR required to update the docs in cpeditor.github.io awaiting for translation:RU Associated PR is waiting for russian translations to be updated awaiting for translation:CN Associated PR is waiting for chinese translations to be updated labels Nov 20, 2020
ouuan
ouuan requested changes Nov 21, 2020
View reviewed changes
Copy link
Member

@ouuan ouuan left a comment •
edited

Choose a reason for hiding this comment

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

  1. I don't like the "create when needed" logic. I'd rather show it when needed and hide it when not available.

  2. I remember that CF Tool fails to submit to URLs with problem code "0" which is in the URL when you enter a running contest, which is the reason I parsed the URL for CF Tool 1.0. I think we can pass the URL to CF Tool only when we fail to parse the URL.

CHANGELOG.md Outdated Show resolved Hide resolved
src/mainwindow.cpp Outdated Show resolved Hide resolved
ui/appwindow.ui Outdated Show resolved Hide resolved
coder3101
coder3101 commented Nov 21, 2020
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@coder3101
Copy link
Member Author

I remember that CF Tool fails to submit to URLs with problem code "0" which is in the URL when you enter a running contest, which is the reason I parsed the URL for CF Tool 1.0. I think we can pass the URL to CF Tool only when we fail to parse the URL.

Yup, I see it is still open.
xalanq/cf-tool#79

What about this solution instead.

If url ends with "0" we make it "A" and submit with that new modified url.

@coder3101 coder3101 requested a review from ouuan November 21, 2020 13:55
coder3101 and others added 3 commits November 21, 2020 19:35
@coder3101
Improve some code
51ca45d
@github-actions @coder3101
style: Format codes (#684)
12c66d4 
Co-authored-by: coder3101 <coder3101@users.noreply.github.com>
@coder3101
Format code in settings.json
9568bfb
ouuan
ouuan reviewed Nov 21, 2020
View reviewed changes
src/Extensions/CFTool.cpp Outdated Show resolved Hide resolved
@coder3101 coder3101 requested a review from ouuan November 21, 2020 14:20
@IZOBRETATEL777 IZOBRETATEL777 removed the awaiting for translation:RU Associated PR is waiting for russian translations to be updated label Nov 22, 2020
@coder3101
Copy link
Member Author

@ouuan Anythine else you want me to update in this PR?

@coder3101 coder3101 removed the awaiting for docs Associated PR required to update the docs in cpeditor.github.io label Nov 22, 2020
@coder3101 coder3101 marked this pull request as ready for review January 30, 2021 03:35
coder3101 and others added 5 commits January 30, 2021 09:07
@coder3101
Merge branch 'master' into submit-in-action-menu
6bf4f87
@coder3101
update translations
2460f39
@coder3101
CFtool: Improve logic and refactor
2a88b5d 
1. Refactor memebers so that they are more readable and sensible
2. Show CFTool error only when submitting the solution, eliminate the
   pre-check
3. Deprecate the use of CF tool < 1.0 as its support should be removed in next
   release cycle. (Reason: CF tool was beta and progressively it added
   URL support and now it is stable and to avoid legacy code to support
   it. We deprecate it now, In next release cycle, if we detech version < 1.0, we fail the submission requesting an update. In this way we do not have to worry about URL parsing and leverage this task to the tool that does the submission)
4. Improve code and re-usability.
5. Submit button will be enabled always, If it is not installed, it will
   show error during submission, Submit button and action will be hidden
   from tabs that do not have CF URL.
@github-actions @coder3101
style: format codes (#758)
cae1d5f 
Co-authored-by: coder3101 <coder3101@users.noreply.github.com>
@coder3101
Update translations
b0958f6
@coder3101
Copy link
Member Author

After this code makes to beta, the next alpha/master will have a commit where we will completely drop the support for CF Tool < 1.0 because of the following:

  1. CF Tool 0.x.x were all beta and prone to breaking changes. 1.0 should be considered at least API/cmd stable.
  2. To avoid having legacy code to Parse URL which can change any day from CF, isn't actually our issue, any such changes should be delegated to CF Tool.

As a result of above, this commit adds a warning to the user that they need to update cf tool to 1.0 or above, as in future release we will not support it.

@coder3101 coder3101 enabled auto-merge (squash) February 6, 2021 16:10
@coder3101 coder3101 requested a review from ouuan February 6, 2021 16:11
@coder3101 coder3101 removed the work in progress The work has been started and is in progress. label Feb 6, 2021
@ouuan ouuan self-assigned this Feb 6, 2021
@ouuan
Copy link
Member

ouuan commented Feb 7, 2021 •
edited

empty contest ID and problem code in the toast message.

Haven't read the code yet. But this problem remains.

@coder3101
Copy link
Member Author

Sure! I will fix it.

@coder3101
Copy link
Member Author

empty contest ID and problem code in the toast message.

Haven't read the code yet. But this problem remains.

Fixed. Please review now!

coder3101
coder3101 commented Feb 10, 2021
View reviewed changes
src/Extensions/CFTool.cpp Outdated Show resolved Hide resolved
coder3101
coder3101 commented Feb 10, 2021
View reviewed changes
src/Extensions/CFTool.cpp Outdated Show resolved Hide resolved
coder3101
coder3101 commented Feb 10, 2021
View reviewed changes
src/appwindow.cpp Outdated Show resolved Hide resolved
@ouuan ouuan self-assigned this Feb 12, 2021
@coder3101 coder3101 mentioned this pull request Mar 7, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ouuan ouuan Awaiting requested review from ouuan

Requested changes must be addressed to merge this pull request.

Assignees

@ouuan ouuan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Sometimes CF tool fails to parse URL Add hotkey for submit button
3 participants
@coder3101 @ouuan @IZOBRETATEL777