Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

P & S preset rotate and hotkey #28

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

Conversation

john-peterson
Copy link
Contributor

Reading rotate P & S preset

Discussed at

@Underground78

Can you explain what it does exactly?

Read three additional PnS parameters from [Settings\PnSPresets], if they are present

I use it for horizontal and vertical flip

[Settings\PnSPresets]
Preset0=Horizontal flip,0.500,0.500,1.000,1.000,0,180,0
Preset1=Vertical flip,0.500,0.500,1.000,1.000,180,0,0

I would guess it break old settings

No

does it mean that this info was saved but never read?

No it's not saved by the P & S dialog because

  • it doesn't have fields for those values

Adding P & S preset toggle key

Name and position

rename the key to "PnS Next Preset"

The key

name is "Next PnS Preset" because

  • it's consistent with the key "Next AR Preset"

position is near "Next AR Preset" because

  • they have a similar purpose of changing a preset

The key is

renamed to "PnS Next Preset" because

  • it's consistent with the other "PnS" keys

moved near the other "PnS" keys because

  • its function is a PnS function as the other "PnS" keys

Previous PnS Preset

why isn't there a "PnS Previous Preset" key?

  • it's consistent with "Next AR Preset" because there isn't a "Previous AR Preset" key
  • it has limited value because all presets can be accessed with a next button

Resource

Discussed at #28 (comment)

Don't remove this in the same patch please. It's a leftover on purpose but I'd rather completely remove it from every file than partially

This line shouldn't be kept

  • if the resouce file is changed from VS rather than directly

because

  • doing so is a manual task after a change in the String Table → Open or Resource Symbols dialog

Changing resource file encoding from UCS-2 to ASCII

Discussed at #28 (comment), #28 (comment), #28 (comment)

When a UCS-2 file is changed the patch with the change will be binary if one file version (in this case the old) is binary. (Subsequent edits will be from text to text.)

The disadvantage of a binary patch is

  • it's larger (it includes the whole file that's changed)
  • can't display the difference with git

git create a text patch for UTF-8 and ASCII

Programs that support additional character sets (and therefore don't rely on git to generate the patch) include

  • TortoiseGitMerge.exe

We need UTF-16 for the translations. Does the resource compiler allow us to mix ASCII and UTF-16?
The resource compiler doesn't work with ASCII

Is there another way to solve the problem?

Fixing ffmpeg gcc i686-w64 host prefix

This commit is in mingw

It's not merged because

  • it's not useful

Commit message

@jeeb

summary line … 50 to around 70 characters

I agree that the first line should be a title of this length

After that … an empty line
… extra paragraphs …72-75 characters

The author shouldn't create line breaks in the commit message because

  • it lets the reader rather than the author decide what the appropriate line length is
  • some commit message readers can wrap text
  • this reasoned discussion support that

@CrossVR
Copy link
Contributor

CrossVR commented Dec 4, 2012

Commit ba56ce2 is dirty, it removes a build configuration from the solution file.

@CrossVR
Copy link
Contributor

CrossVR commented Dec 4, 2012

I don't think MSVC works well with UTF-8 resource files, Underground78 or ��XhmikosR� can tell you more about that.

@Nevcairiel
Copy link
Contributor

Its correct, resource files need to either be ASCII, or if you need more then the 7-bit ASCII provides, UTF-16.

@XhmikosR
Copy link
Contributor

XhmikosR commented Dec 4, 2012

Exactly, UTF8 doesn't work.

@XhmikosR XhmikosR closed this Dec 4, 2012
@XhmikosR
Copy link
Contributor

XhmikosR commented Dec 4, 2012

And as we have already discussed in the past, people didn't like the delete option. Personally, I see when it can be useful, but it's what most people prefer.

(Re-opening the PR since I accidentally closed it)

So, clean up your patches, and force push. john-peterson@a47923c and john-peterson@3a30d1f should be removed completely from the PR.

And please run docs/run_astyle.bat before force pushing.

@XhmikosR XhmikosR reopened this Dec 4, 2012
@@ -225,7 +228,6 @@
#define ID_FAVORITES_ORGANIZE 937
#define ID_FAVORITES_ADD 938
#define ID_HELP_HOMEPAGE 939
//#define ID_HELP_DOCUMENTATION 940
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't remove this in the same patch please. It's a leftover on purpose but I'd rather completely remove it from every file than partially.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm aware of this, but that's why you are supposed to verify the changes before you push. Don't remove these for now.

@jeeb
Copy link
Contributor

jeeb commented Jan 31, 2013

General comment about commit messages:

While in general your commit messages are nice, I would try to keep commit messages' line lengths under control, especially for the summary line -- preferably somewhere between 50 to around 70 characters. After that you can add an empty line and then start the possible extra paragraphs for more detailed explanation etc. with a line length of around around 72-75 characters.

This makes you create a short yet descriptive description/summary on the first line, and then lets you express yourself in a longer way after that. A similar style is mentioned here, but I prefer to have the possibility of having up to 70 or so characters in the summary.

@XhmikosR
Copy link
Contributor

  1. ID_HELP_DOCUMENTATION was recently removed.
  2. The resource compiler doesn't work with ASCII, it's so simple. So that change cannot go in.

@CrossVR
Copy link
Contributor

CrossVR commented Feb 12, 2013

We've changed the resource file encoding to ASCII while keeping the translations in UCS-2 and updating the translation scripts to handle this. So far it's working well, you can remove e28d17229a0334d051c5f0014082e4401c2b922e.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
5 participants