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

Add scaling parameter #9493

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

Conversation

kevin147147
Copy link
Contributor

This is how are pull requests handled by FreeRDP

  1. Every new pull request needs to build and pass the unit tests at https://ci.freerdp.com
  2. At least 1 (better two) people need to review and test a pull request and agree to accept

Preparations before creating a pull

  • Rebase your branch to current master, no merges allowed!
  • Try to clean up your commit history, group changes to commits
  • Check your formatting! A clang-format script can be found at .clang-format
    • The cmake target clangformat reformats the whole codebase
  • Optional (but higly recommended)
    • Run a clang scanbuild before and after your changes to avoid introducing new bugs
    • Run your compiler at pedantic level to check for new warnings

To ease accepting your contribution

  • Give the pull request a proper name so people looking at it have an basic idea what it is for
  • Add at least a brief description what it does (or should do :) and what it's good for
  • Give instructions on how to test your changes
  • Ideally add unit tests if adding new features

What you should be prepared for

  • fix issues found during the review phase
  • Joining our chat to talk to other developers or help them test your pull might accelerate acceptance
    • Matrix room : #FreeRDP:matrix.org (main)
    • XMPP channel: #FreeRDP#matrix.org@matrix.org (bridged)
    • IRC channel : #freerdp @ irc.oftc.net (bridged)
  • Joining our mailing list freerdp-devel@lists.sourceforge.net may be helpful too.

Please remove this text before submitting your pull!

@freerdp-bot
Copy link

Can one of the admins verify this patch?

Copy link
Member

@akallabeth akallabeth left a comment

Choose a reason for hiding this comment

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

Overall ok, but some things missing.
also, please run git clang-format before committing a change, it will fail the ci builder otherwise.

@@ -38,6 +38,7 @@ public class BookmarkDB extends SQLiteOpenHelper
static final String DB_KEY_SCREEN_RESOLUTION = "resolution";
static final String DB_KEY_SCREEN_WIDTH = "width";
static final String DB_KEY_SCREEN_HEIGHT = "height";
static final String DB_KEY_SCREEN_SCALE = "scale";
Copy link
Member

Choose a reason for hiding this comment

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

you need to increment the database version and add an update path for the new field.

android:entries="@array/scale_array"
android:entryValues="@array/scale_values_array"
android:key="bookmark.scale"
android:summary="Scaling Percentage"
Copy link
Member

Choose a reason for hiding this comment

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

  • no hard coded strings, move to strings.xml.
  • Summary is not really helpful, this is HighDPI support, so remote scaling, not local /smart-sizing like

android:entryValues="@array/scale_values_array"
android:key="bookmark.scale"
android:summary="Scaling Percentage"
android:title="@string/settings_scale" />
Copy link
Member

Choose a reason for hiding this comment

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

missing in strings.xml

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.

None yet

3 participants