URLリンクの改善 #566

berryzplus · 2018-10-19T16:42:07Z

#563

サブクラス化と同時に適切なサイズにリサイズする処理を追加

ds14050 · 2018-10-19T17:33:55Z

issue の方の解説がたいへん参考になりました。ところで、今度は短すぎやしませんか？

sakura_core/dlg/CDlgAbout.cpp

berryzplus · 2018-10-20T02:49:08Z

全体の幅が狭い件についてですが、いまは描画がはみ出さないギリギリを指定してます。

システムからボーダーの幅と高さを持ってきてマージン作ってやれば少しましになるかも知れません。

m-tmatma · 2018-10-20T10:28:54Z

#570 を対応してみました。

サブクラス化と同時に適切なサイズにリサイズする処理を追加

berryzplus · 2018-10-21T11:28:22Z

#569 マージしたのでリベースしました。

わがままですが #568 対応のために #578 のマージを待ちたい感じです。
（#578 = デバッグ版で点が出る件の対応）

いまのPRには #578 のコミットを含んでいるので、appveyorからは最終形（のつもり）が取得できるはずです。

使わないグリフ配列のためのメモリ確保をやめる最大幅の指定を0xFFFF→SHRT_MAXに変更エラー処理を追加（エラーの場合はサイズ変更しない）描画処理のリテラル2をDPI考慮した値に変更コントロールのマージン領域を左だけでなく右にもとるように変更コメントを追記

berryzplus · 2018-10-28T09:23:55Z

#578の対応に疑義が出たので、このPRからは外すことにしました。

再レビューをお願いします。

sakura_core/dlg/CDlgAbout.cpp

ds14050 · 2018-10-28T16:15:09Z

GetCharacterPlacement は求めるものに比して高機能すぎる気もしますが、動作結果には問題がありません。フォントと DPI に応じて正しい幅が得られているようです。

↓ ＭＳＰゴシックも低解像度向けのビットマップフォントでなければいいものだ。

ds14050

待ちましたが他の方の意見が出ないので Approve してしまいます。

コードに関しては流れだけ見ました。フォントの途中変更に対応していない(コントロールのフォントではなくメンバ変数の m_hFont に依存している)のが気になりましたが、それをいうと下線を付けるという従来の処理も同じ問題を抱えているので見なかったことにします。問題が出ればまとめて対応できるでしょう。

m-tmatma · 2018-11-02T13:13:36Z

sakura_core/dlg/CDlgAbout.h

@@ -31,8 +31,12 @@ class CUrlWnd
 virtual ~CUrlWnd() { ; }
 BOOL SetSubclassWindow( HWND hWnd );
 HWND GetHwnd() const{ return m_hWnd; }
+protected:


protected を複数存在させているのはなぜですか?

既存のコードは、メンバ関数とメンバー変数で protected を
複数指定しているのだと思いますが、protected の指定を
足す必要はないように思います。

C++でのprotectedはただのラベルです。
何個あってもよいですし、なくてもいいです。

個人の傾向としてはメンバの区切りとしていれることもあります。
単に個数の話であれば、気にしなくてよいのでは？と思います。

何個あってもよいですし、なくてもいいです。

ないのとあるのは違います。

個人の傾向としてはメンバの区切りとしていれることもあります

どのような区別をするために入れていますか?

外部に公開するメソッド(public)と内部で使うメソッド(protected)で分けていますね。
38行目のprotectedはstaticなディスパッチャ関数とメッセージハンドラ関数を区別するために入れてます。

どういう使い分けをしているのかをコメントに記載したほうがいいです。
書いた人以外には伝わらないので、誰が別の人が変更した場合
バラバラになる可能性大です。

メソッド毎に指定したいなら思い付きですが下記のようにするのはどうでしょうか？

#define PUBLIC public:\ #define PROTECTED protected:\ #define PRIVATE private:\ class DamnedWar: { PUBLIC void enemy(); PROTECTED void human_shield(); PRIVATE void military_company(); };

#define 無しなら私は拒絶しませんよ。あえて Java や C# に似せるために手間をかけることもなかろうと実行はしませんでしたが、考えたことはあります。

メソッド毎に指定したいなら

これは少し違っています。

普通に構造化した関数を書くと↓のような感じになると思うんです。

public:
公開の入り口メソッド（）；
protect:
内部的な準備をする部分（）；
内部的な実処理をする部分（）；
内部的な後始末をする部分（）；
private:
内部共通処理１（）；
内部共通処理２（）；
内部共通処理３（）；

このクラスについて書くと、
ウインドウを表す、というクラスの特殊性から変則的な枠組みが出てきます。
staticのプロシージャ関数とprotectedなイベントハンドラ群が追加されます。

（誤爆して違うとこに貼ったので貼り直し）

うーん、protected のラベルをあえて増殖させる理由が分かりませんが、元の変更前のコードから既にメソッドとメンバー変数で複数 protected のラベルを使ってるんですね。そして @berryzplus さんのこのPRでは protected なメソッドでも何か区分けをしたかったのか（上のコメントに書かれていますが）更に追加をしたと。

しかし CUrlWnd ってそこから派生した class が無いんだし final 付けて、protected は private に変えちゃった方が変な憂いが無くなるのではと better C 的に C++ を使っている人からすると思いました。そんでもって m_hFont メンバー変数は GetFont メソッドをわざわざ用意せずに直接アクセスした方が手っ取り早いのでは、とか思いましたが、まぁ色々ひっくるめて細かい事は気にしない方が良いですね。

概ね beru さんに同意です。最後の一文も含めて。

public はクラス利用者への公開インタフェイス。protected はクラス継承者への公開インタフェイス。private はクラスのプライベート。public と protected の混在はそれだけで単一責任原則に違反しているので解消すべき、というどこかからの受け売りを信条にしています。

m-tmatma · 2018-11-02T13:19:16Z

sakura_core/dlg/CDlgAbout.cpp

+ const INT nMaxExtent = SHRT_MAX; // 幅計算できる最大幅を指定
+
+ auto vxBuf = std::make_unique<INT[]>( cchText );
+ auto vDx = vxBuf.get();


vDx の変数名はどういう意味ですか?

vector of DXです。

DXは小学生男子のあこがれ・・・ちがう。

GDI関数のマニュアルで、横方向の論理単位を表す引数名によく使われている呼称です。
おそらく、Device Xの頭文字をとったものと思われます。
中でもDXの配列は、文字列を描画するときに使う、文字幅を保持した配列であることが多いようです。
この辺、本当のところはマイクロソフトのエロい人を小一時間ほど問い詰めてあげてください・・・

https://docs.microsoft.com/en-us/windows/desktop/api/winuser/nf-winuser-scrolldc

ぐぐったところ上記の ScrollDC の解説ページに辿り付きました。
引数の dx の解説を見る限り d というのは device を表している可能性が微？

マニュアルに device unit って書いてありました。

sakura_core/dlg/CDlgAbout.cpp

m-tmatma · 2018-11-02T13:20:59Z

sakura_core/dlg/CDlgAbout.cpp

+ auto vxBuf = std::make_unique<INT[]>( cchText );
+ auto vDx = vxBuf.get();
+
+ auto cchGlyph = ( cchText * 3 / 2 ) + 16; // エラーグリフの増分を加味したグリフ数を指定


この計算式のロジックを教えてください。

このページの解説部分を参照、です。

このため pwOutGlyphs パラメータに指定するバッファのサイズは、文字バッファの長さの 1.5 倍に、無効な文字シーケンスがあった場合などの特殊なケースのための 16 グリフ程度を加えたサイズにしておくのが適切です。

いかなるケースにも対応可能かどうかは不明ですが、そのまま実装してあります。

ありがとうございます。
示していただいたリンクは ScriptShape に対する説明ですが、
呼び出している関数は GetCharacterPlacement です。
ScriptShape とGetCharacterPlacement はどう関係してきますか?

ScriptShape とGetCharacterPlacement はどう関係してきますか?

詳細と概要の関係です。

似た関係性としてI/O関数の低水準入出力と高水準入出力があります。
「1行読む」という処理は「1文字読む」の集まりです。

GetCharacterPlacementは、裏でuniscribeというコンポーネントを使うと以前書きました。

ScriptShapeはuniscribeの一部で、文字列からグリフ配列を生成する部分を担当します。

実際に呼出しされているかどうか検証してみたことはありませんけど 😄

メモ

Uniscribe 関連の URL
https://docs.microsoft.com/en-us/windows/desktop/intl/uniscribe
https://docs.microsoft.com/en-us/windows/desktop/Intl/uniscribe-reference
https://docs.microsoft.com/en-us/windows/desktop/intl/using-uniscribe

GetCharacterPlacement の使い方を理解するところで止まっていて放置してました。
GetCharacterPlacement の使い方って API リファレンス以外で参考にしたものはありますか?

特定のサイトではないですね。

ExtTextOutと組み合わせて使うもので、電子活版印刷という考え方があることを知るまでに少し時間がかかった気がします。「豆大福おいしい」と書かれたブログで見た日本語活版印刷に関する考察の記事が印象に残っています。

m-tmatma · 2018-11-02T13:28:38Z

指摘ではないですが、
case 文のところに宣言なんて、最近のコンパイラではこんな書き方ができるのか!

sakura/sakura_core/dlg/CDlgAbout.cpp

Lines 446 to 452 in 8520f75

 case WM_PAINT: 

 // ウィンドウの描画 

 PAINTSTRUCT ps; 

 HFONT hFont; 

 HFONT hFontOld; 

 TCHAR szText[512];

sakura_core/dlg/CDlgAbout.cpp

ds14050 · 2018-11-02T14:22:14Z

case 文のところに宣言

往々にして変数名が衝突するので、スコープを ~~case 文~~個々の case ラベルに限定するためにカーリーブラケットを付けますね。そのインデントが break の位置と相まってまた紛争の種に……。

beru · 2018-11-02T18:04:34Z

case 文のところに宣言

往々にして変数名が衝突するので、スコープを case 文に限定するためにカーリーブラケットを付けますね。そのインデントが break の位置と相まってまた紛争の種に……。

これは私が長年の研鑽により編み出した秘技ですが惜しまずに公開します。

#define BEGINE {
#define ENDO }

beru · 2018-11-02T18:19:36Z

指摘ではないですが、
case 文のところに宣言なんて、最近のコンパイラではこんな書き方ができるのか!

sakura/sakura_core/dlg/CDlgAbout.cpp

Lines 446 to 452 in 8520f75

case WM_PAINT:

// ウィンドウの描画

PAINTSTRUCT ps;

HFONT hFont;

HFONT hFontOld;

TCHAR szText[512];

言語仕様やそのBNFを理解はしていませんがぐぐったところ以下の解説が見つかりました。
https://stackoverflow.com/a/92439/4699324

case 文はラベル相当のようです。なのでスコープとしては switch 文の { } の内側のスコープに充たるのかもしれないですね。なお色々なコンパイラで試してはいません。

bRet →　retSetText 標準のSetTextの戻り値 vxBuf → dxBuf DX配列保持バッファ vDx → pDx DX配列を参照するポインタ

berryzplus · 2018-11-03T03:44:33Z

指摘いただいた点について修正を加えました。

都合：　分かりづらいのでコメントを追加
vxBuf、vDx：　より適切な名前に変更

m-tmatma · 2018-11-04T02:06:05Z

指摘いただいた点について修正を加えました。

ありがとうございます。

m-tmatma · 2018-11-10T13:40:55Z

放置していたので、PR のリビルドをかけた
https://ci.appveyor.com/project/sakuraeditor/sakura/builds/20196291

berryzplus

放置されてたコメントに気付いたので投稿します。
結構前に書いたコメントなはず...

berryzplus · 2018-11-08T15:39:55Z

sakura_core/dlg/CDlgAbout.h

@@ -31,8 +31,12 @@ class CUrlWnd
 virtual ~CUrlWnd() { ; }
 BOOL SetSubclassWindow( HWND hWnd );
 HWND GetHwnd() const{ return m_hWnd; }
+protected:


外部に公開するメソッド(public)と内部で使うメソッド(protected)で分けていますね。
38行目のprotectedはstaticなディスパッチャ関数とメッセージハンドラ関数を区別するために入れてます。

berryzplus · 2018-11-11T14:37:31Z

このPRは既に障害が消えていると思っています。
再レビューをお願いしたいです。

ds14050 · 2018-11-11T15:04:39Z

再レビューをお願いしたいです。

今バトンを握っているのは @m-tmatma さんです。

berryzplus · 2018-11-12T00:22:55Z

ちょっと確認ですが、PRの可否判断にどうしても必要な情報ですか？

「何をしているか分かりません」なら回答する意味があると思います。しかしそうじゃないですよね。明らかに何が別なことを気にした質問を繰り返しているように感じています。単なる質問ならマージ後でも受け付けます。可否判断をなるべく早く下すようにお願いしたいです。

いま必要 or not ということです。

m-tmatma · 2018-11-12T00:25:52Z

api の仕様を理解できてないので
理解するための情報がほしいだけです。

だれか他に理解されている方がいて
問題ないという話であれば、マージしてもいいです。

berryzplus · 2018-11-12T03:36:20Z

了解しました。

いずれ必要になるので説明はしますが、いまここにgetcharacterplacementの高機能は必要ないので、利用方法が直感的で利用実績もあるdrawtextを使って書きなおすようにします。

なる早のつもりですがしばらくお待ちください。

…size_on_bind

berryzplus · 2018-11-12T16:50:45Z

対応してみました。

GetCharacterPlacement → DrawTextです。
マニュアルを見れば分かる通り、これもかなり色々なことができる関数ですが、DT_CALCRECTだけを渡すシナリオでは指定した文字列がちょうど収まる矩形を求める単純関数になります。

りべーすしなくてもmasterの変更を取り込めるのか試してみたくて、マージしてみたんですが想像したのとちょっと違うコミットが作られていて「えええ・・・」です。マージすると消えるのかな・・・。これについては、よく分っとらんです。

ds14050 · 2018-11-12T17:10:29Z

りべーすしなくてもmasterの変更を取り込めるのか試してみたくて、マージしてみたんですが想像したのとちょっと違うコミットが作られていて「えええ・・・」です。マージすると消えるのかな・・・。これについては、よく分っとらんです。

master の変更がこの PR の内容の一部であるかのようにとりこまれているが、マージする際には共通コミットとして安全に無視される、という雰囲気を感じました。結果的には問題ないものの論理的には PR に不純物が混じっているような。あくまでも雰囲気ですが。

ds14050 · 2018-11-12T18:16:00Z

動作を確認しました。フォントと DPI 共に適応できています。

sakura_core/dlg/CDlgAbout.cpp

m-tmatma · 2018-11-13T13:15:52Z

いずれ必要になるので説明はしますが、いまここにgetcharacterplacementの高機能は必要ないので、利用方法が直感的で利用実績もあるdrawtextを使って書きなおすようにします。

すごくわかりやすくなりました。
ありがとうございます。

一つ質問を書きました。

beru

動作確認しましたが問題無いと思います。

m-tmatma · 2018-11-17T08:40:25Z

@berryzplus さん、マージどうぞ

berryzplus · 2018-11-17T08:44:05Z

@beru さんのこの画像が気になっているのです。

よく見るとカーソルの下のCUrlWndが2つ同時に色変わってるんです。
この修正の内容には関係ないと思ってるんですが、ちょっと調べたくて逡巡しています、

beru · 2018-11-17T08:53:48Z

よく見るとカーソルの下のCUrlWndが2つ同時に色変わってるんです。
この修正の内容には関係ないと思ってるんですが、ちょっと調べたくて逡巡しています、

そのスクリーンショットは表示のタイミングがシビアで PrintScreen キー押しでは取れませんでした。
ScreenToGif で動画を記録してその中からちょうど良く映っているコマの絵を保存しました。
同期した動作にしないと首をはねられるというわけではないので放置で良いと思います。

berryzplus · 2018-11-17T10:20:14Z

マージします。

…n_bind URLリンクの改善

m-tmatma reviewed Oct 19, 2018

View reviewed changes

sakura_core/dlg/CDlgAbout.cpp Outdated Show resolved Hide resolved

m-tmatma reviewed Oct 19, 2018

View reviewed changes

sakura_core/dlg/CDlgAbout.cpp Outdated Show resolved Hide resolved

arigayas mentioned this pull request Oct 20, 2018

デバッグ版の「バージョン情報」のURL部分の1文字目より前の左上にドットがある #568

Closed

berryzplus mentioned this pull request Oct 20, 2018

バージョン情報ダイアログの整理 #569

Merged

URLリンクの改善

c4f91c4

サブクラス化と同時に適切なサイズにリサイズする処理を追加

berryzplus force-pushed the feature/resize_on_bind branch from 43d0646 to d15d346 Compare October 21, 2018 11:17

berryzplus force-pushed the feature/resize_on_bind branch from 6397ceb to e547bcb Compare October 21, 2018 16:04

berryzplus added 2 commits October 27, 2018 19:29

CUrlWndがWM_SETSIZEを処理するように改造

8520f75

berryzplus force-pushed the feature/resize_on_bind branch from e547bcb to 8520f75 Compare October 27, 2018 10:29

ds14050 reviewed Oct 28, 2018

View reviewed changes

sakura_core/dlg/CDlgAbout.cpp Outdated Show resolved Hide resolved

ds14050 previously approved these changes Nov 1, 2018

View reviewed changes

m-tmatma reviewed Nov 2, 2018

View reviewed changes

sakura_core/dlg/CDlgAbout.cpp Outdated Show resolved Hide resolved

m-tmatma reviewed Nov 2, 2018

View reviewed changes

sakura_core/dlg/CDlgAbout.cpp Show resolved Hide resolved

変数名修正とコメント追加

c7d5297

bRet →　retSetText 標準のSetTextの戻り値 vxBuf → dxBuf DX配列保持バッファ vDx → pDx DX配列を参照するポインタ

berryzplus dismissed ds14050’s stale review via c7d5297 November 3, 2018 03:42

berryzplus commented Nov 10, 2018

View reviewed changes

berryzplus added 2 commits November 13, 2018 01:26

サイズ計測を簡易化する

8144843

Merge remote-tracking branch 'remotes/project/master' into feature/re…

528e617

…size_on_bind

m-tmatma reviewed Nov 13, 2018

View reviewed changes

sakura_core/dlg/CDlgAbout.cpp Show resolved Hide resolved

beru approved these changes Nov 16, 2018

View reviewed changes

ds14050 mentioned this pull request Nov 16, 2018

改行記号の文字色が RGB(0,0,0) の設定時に、ブロックコメント内の改行記号がコメントの文字色と同じ色で表示される #611

Open

m-tmatma approved these changes Nov 17, 2018

View reviewed changes

berryzplus merged commit 1d1dcbc into sakura-editor:master Nov 17, 2018

berryzplus deleted the feature/resize_on_bind branch November 17, 2018 10:21

beru mentioned this pull request Nov 27, 2018

「バージョン情報」のURL部分にマウスカーソルを持っていったときの背景色が長い #563

Closed

m-tmatma added this to the next release milestone Dec 1, 2018

HoppingTappy pushed a commit to HoppingTappy/sakura that referenced this pull request Jun 11, 2019

Merge pull request sakura-editor#566 from berryzplus/feature/resize_o…

72d9ceb

…n_bind URLリンクの改善

URLリンクの改善 #566

URLリンクの改善 #566

Conversation

berryzplus commented Oct 19, 2018

ds14050 commented Oct 19, 2018

berryzplus commented Oct 20, 2018

m-tmatma commented Oct 20, 2018

berryzplus commented Oct 21, 2018

berryzplus commented Oct 28, 2018

ds14050 commented Oct 28, 2018 • edited Loading

ds14050 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ds14050 Nov 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-tmatma commented Nov 2, 2018

ds14050 commented Nov 2, 2018 • edited Loading

beru commented Nov 2, 2018 • edited Loading

beru commented Nov 2, 2018

berryzplus commented Nov 3, 2018

m-tmatma commented Nov 4, 2018

m-tmatma commented Nov 10, 2018

berryzplus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berryzplus commented Nov 11, 2018

ds14050 commented Nov 11, 2018

berryzplus commented Nov 12, 2018

m-tmatma commented Nov 12, 2018

berryzplus commented Nov 12, 2018

berryzplus commented Nov 12, 2018

ds14050 commented Nov 12, 2018

ds14050 commented Nov 12, 2018

m-tmatma commented Nov 13, 2018

beru left a comment

Choose a reason for hiding this comment

m-tmatma commented Nov 17, 2018

berryzplus commented Nov 17, 2018

beru commented Nov 17, 2018

berryzplus commented Nov 17, 2018

ds14050 commented Oct 28, 2018 •

edited

Loading

ds14050 left a comment •

edited

Loading

ds14050 Nov 12, 2018 •

edited

Loading

ds14050 commented Nov 2, 2018 •

edited

Loading

beru commented Nov 2, 2018 •

edited

Loading