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

#2717: Improve qt-gui error message popup #2752

Merged
merged 2 commits into from Jun 10, 2019

Conversation

Projects
None yet
3 participants
@0003088
Copy link
Contributor

commented Jun 5, 2019

Basics

Check relevant points but please do not remove entries.
Do not describe the purpose of this PR in the PR description but:

  • Short descriptions should be in the release notes (added as entry in
    doc/news/_preparation_next_release.md which contains _(my name)_)
    Please always add something to the the release notes.
  • Longer descriptions should be in documentation or in design decisions.
  • Describe details of how you changed the code in commit messages
    (first line should have module: short statement syntax)
  • References to issues, e.g. close #X, should be in the commit messages.

Checklist

Check relevant points but please do not remove entries.
For docu fixes, spell checking, and similar none of these points below
need to be checked.

  • I added unit tests
  • I ran all tests locally and everything went fine
  • affected documentation is fixed
  • I added code comments, logging, and assertions (see Coding Guidelines)
  • meta data is updated (e.g. README.md of plugins and METADATA.ini)

Review

Reviewers will usually check the following:

Labels

  • Add the "work in progress" label if you do not want the PR to be reviewed yet.
  • Add the "ready to merge" label if the build server is happy and also you
    say that everything is ready to be merged.

I'ts been a while since my last merge request, so I'm not exactly sure what to do and what items to check, so please advise.

I think the error popup now behaves as desired, however, I have a strange problem, see screenshot. I'm not sure if it's related to the fact, that I use i3 as window manager or if it's an issue with the Qt version on my machine. Could anybody check if this problem is present on their installations?

Screenshot_20190605_210317

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

@0003088 thank you very much for this improvement! You need to

  1. write a line of release notes, followed by _(your name)_., so that your contribution is not forgotten in the release notes
  2. maybe reformat the code with clang-format (the build server gives you the patch and how to apply it if it is difficult for you to install clang-format)

@Piankero can you try this PR and see if the error message looks fine?

@markus2330 markus2330 requested a review from Piankero Jun 5, 2019

@Piankero

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

qt-error-new
Looks good to me :) well done

@Piankero
Copy link
Contributor

left a comment

If you make the build server happy I think we can merge this

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2019

@Piankero can you maybe help with that?

@Piankero

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2019

I can reformat it and add a message to the news. But I need to know the name of @0003088

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2019

Thank you!

doc/AUTHORS.md tells you that his name is "Raffael Pancheri"

@Piankero

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

@markus2330 feel free to merge, the failed linux build is just because fish has an invalid cert for their website atm

@markus2330 markus2330 merged commit 3624c12 into ElektraInitiative:master Jun 10, 2019

8 of 12 checks passed

linux Task Summary
Details
mac KDB_DB_FILE:default.mmap KDB_DB_INIT:elektra.mmap KDB_DEFAULT_STORAGE:mmapstorage Task Summary
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
LGTM analysis: C/C++ No new or fixed alerts
Details
bsd FreeBSD:freebsd-11-2-release-amd64 Task Summary
Details
bsd FreeBSD:freebsd-12-0-release-amd64 Task Summary
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
mac Task Summary
Details
mac ASAN_OPTIONS:detect_leaks=1 BINDINGS:cpp ENABLE_ASAN:ON TOOLS:kdb Task Summary
Details
mac BUILD_FULL:ON BUILD_SHARED:OFF Task Summary
Details
@markus2330

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

Thank you very much!

invalid cert for their website atm

Yes, the URL is already fixed in master, so a rebase would also avoid the problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.