- User Since
- Mar 7 2016, 5:11 PM (167 w, 6 d)
This diff was on hold because of the work I am doing on the new annotation toolbar, that may make all the annotation configuration dialogs obsolete. But I now think those dialogs may still be useful, so that we can move this review forward. I am going to rebase it and possibly fix the layout of the newly added fields in a few days.
I am going to rebase it to master and push some new changes in a few days.
Sat, May 18
Wed, May 15
With the Alt key the bug does not appear, because the existing code disables correctly the browse mode when Alt is released. For some reasons key press and release events are different for Ctrl key but are the same for Alt. Probably the method avoidMenuAltFocus manages this behavior correctly for the Alt key (I have not analyzed in detail what it does) and then we enter correctly in the if at line 230 where source browse mode is disabled. So removing the redundant Ctrl key solves the bug completely (at least I haven't manage to reproduce it).
- Disable Ctrl and keep only Alt
Sun, May 12
Mar 12 2019
Mar 11 2019
Mar 1 2019
@ngraham Thanks for the guidelines
Feb 28 2019
I never landed a diff before because I just got my KDE developer account.
Few general questions:
- Is it enough that one reviewer accept the revision, or all the listed ones should accept it? (or depends on the project?)
- Can I directly land it once the condition above is satisfied, or is it always better to ask before landing it?
Feb 25 2019
Feb 24 2019
Feb 20 2019
- Revert lessons ids
- Substitude non ASCII characters
Feb 10 2019
Note that the uuid of the course is the same while the uuids of the lessons have changed. Not sure what should be changed and what not.
Jan 18 2019
Maybe @andreask knows what are the most suitable icons to use.
Jan 17 2019
Yes, that is what I meant.
Probably the collapsed by default (current situation) is ok if there is the button.
I use the side review panel only when I need to go through all the reviews one by one, so it is annoying having to expand all the elements by hand. Maybe a expand/collapse all solution would be a better solution. (The long answer is in T8553 ).
Elaborating a bit on why I proposed some changes based on my main usage pattern.
Jan 16 2019
I have not even started working on this task, so your help is more than welcome. I agree that may be useful discussing the points before writing too much code. In particular, I think that the architecture of the proxy model should be done properly given that it is the culprit of some points in the list. For me it was not straightforward how to implement it properly.
Jan 2 2019
Ok, I think it should be ok now.
- Add lesson with all accented letters, better word balance for rare words
I would like to add few improvements to this lesson (to fix few problems I am noticing only now), so I ask you to wait before working on this review.
I completely agree with you, having two different lessons can be beneficial.
- Fix lesson title and description
Oct 14 2018
Oct 13 2018
Oct 9 2018
Oct 8 2018
Oct 4 2018
Current situation on a 1680px wide screen:
Should compile now. Typewriter tool is not included yet. I add few more actions but the color picker works only for text annotation tools for now.
- Check if chosen color is valid before storing it
- Notify PageViewAnnotator when the color has changed
- Add missing annotation tools actions
- Add XML annotation tools and connect corresponding actions
Sep 18 2018
Another UI problem to solve is how to deal with some specific annotation tools "advanced" configurations:
- Some tools have an icon (popup note, stamp)
- Straight line tool has Line extensions
- Inline note has font, font size, alignment
Yours should go first. I think the toolbar thing is going to take some time both because I'll work on it intermittently and because it may require many changes under the hood. Let's see if I can manage to get it ready for KDE Applications 18.12.
Sep 17 2018
@tobiasdeiminger ..and yes I would like to coordinate so we don't waste effort.
@tobiasdeiminger mentioned in D15580 that long pressing to select different geometrical shapes (polygon, ellipse) is not very intuitive. I agree on this. I implemented it that way to be consistent with the other tool buttons of okular, for example the selection button (text selection, table selection, ...). In particular the buttons are based on ToolAction which derives from KSelectAction. I would change the default button mode of ToolAction to QToolButton::MenuButtonPopup, but I believe we can open a new bug for this and do it for all Okular buttons.
I have added the revision D15580 where I implemented a first (very rough) version of the toolbar, so we can start discussing the best way to implement it.
To test this code temporary move .config/okularpartrc or the annotation tools may not match the ones in the buttons.
Sep 12 2018
Just tested again with my solution on top of sander one and the bottom border disappears, but not the upper one. Strange, I didn't remember this effect when I tested it months ago.
I arrive a little late, but testing Okular today I noticed a minor bug with your solution. If you slowly scroll the page by dragging it with the browse tool the upper/lower dark border are not drawn sometimes when the word is at the border of the page. See bug 398553. With the "solution" I proposed instead this was not happening IIRC.
Sep 10 2018
Much better now. Thanks!
Sep 8 2018
I was thinking that a way to facilitate a newcomer getting into a project could be to find him a code buddy, another newcomer working on the same project.
May 16 2018
The link to the Mentoring page is literally the last word of the Getting involved page. I did read the getting involved page (maybe with not too much attention), but I did not reach the mentoring page and did not figured out I could contact a developer directly. Maybe this link can be highlight better somehow?
May 15 2018
May 13 2018
@andreask Thanks for the quick reply. I'll soon make a list of the icons required and let you know.
May 12 2018
Here are my 2 cents.
May 11 2018
Ok, thanks for testing again. I re-tried your patch with the latest master (just to check that there weren't modifications that solved the problem) and I still can reproduce the error. With my modification it goes away. So let's wait for aacid to review the diff.
This is a first mockup of the annotation toolbar.
May 10 2018
Have you tried also what I suggested to reproduce it? i.e. Position an highlighted word that match the search at the top boundary of the view so that it is half visible and then scroll to show it completely. (Just to confirm this is not reproducible)
May 9 2018
@sander Have you had the time to try the fix I suggested?
- Focus text edit of annotation when double-clicking on annotation icon
Apr 24 2018
I'll have a look at it. I think that focusing it makes sense also to locate which popup note is associated with the annotation clicked (especially if they are not overlapped).
Apr 22 2018
Indeed I noticed that analyzing the code. I'll try to rewrite the proxies to solve these problems.
Apr 19 2018
- Propagate dataChanged in Author and Page proxy models
Apr 18 2018
- Propagate the event
Apr 7 2018
You're not really pushing a "branch" with arc, you can think of it more as a diff describing a range of commits.
This review was added by mistake and can be deleted.
I have created a new revision D12013 by mistake. That can be deleted.
- Remove extra space
- Remove extra space
Add suffix "with comment" instead of asterisk for more clarity