- User Since
- Apr 15 2017, 7:18 PM (45 w, 10 h)
Thanks for the patch! Reading the --help text, it seems like this is an option designed to be run in conjunction with --background` mode:
Maybe send an email to firstname.lastname@example.org?
Yes, that would be in a separate diff. If you'd like, implement the feature in Dolphin for now, and then we can remove it when your KIO patch lands. Otherwise we'll have to delay this by a few weeks or more, waiting for whatever KDE Frameworks version your KIO patch lands in.
We still use Bugzilla to track bugs and small tasks; see https://community.kde.org/Infrastructure/Phabricator
No need for a space between FEATURE and the :
In what way does it break Oxygen? What's missing?
Make sure it compiles before pushing, please. Now it no longer does for me:
Thanks for the patch! Couple of notes:
Can confirm, this definitely improves things with overlapping notes.
Looks like not. Landing it.
Oops, looks like your last update replaced the original change.
No need to abandon the revision, you can just mark it as "Changes Planned".
Fri, Feb 23
Awesome patch; this is gonna look so good soon.
Much better! Hope you don't mind that I re-arranged your screenshots; it's easier to see the differences between befores and afters by switching between them rapidly using the left and right arrow keys if they're adjacent.
+1, get rid of that thing!
In fact, Spectacle already has "click on a window to select it" mode.
Hmm, in the After Normal screenshot, I don't like how there's now no line or other visual separation between the footer and the new toolbar. The two elements seem to blend into one another. I kind of liked it better before, IMHO.
@fahadalsaidi, I recently migrated an older patch of yours from Reviewboard to Phabricator (D10298), but I just closed it in favor of this one. If D10298 is still relevant even with this patch, please commandeer and re-open it. Thanks!
Thanks for the test! However, the original author of this patch recent re-appeared here on Phabricator and submitted a better one: D10455: Add RTL support for search, copy & paste in pdf.
OK cool! Feel free to close this if the results of that discussion prove fruitful.
No worries, you're all good!
Up to you; I don't have strong feelings on the matter.
Do any Frameworks folks want to chime in before I land this?
Is the screenshot in the Summary section still accurate? I'd like to feature this in the coming weekend's Usability & Productivity blog post (assuming it lands today or tomorrow; if not, next week's post), and people love looking at screenshots.
Thu, Feb 22
Use the right branch
OK, the i18n error is gone now. But do we really need that "also available in XXXX" text at the end of the Description, anyway? Apparently nobody reads it. Maybe we should focus on polishing the main source chooser UI and get rid of that little vestigial one.
I see what you're going for here, but the "Sources" menu you're trying to add still doesn't actually work, same as the Sort menu. Is there an open Kirigami patch that this depends upon?
The title is still the primary focus for this header UI, which means (for RTL languages at least), it needs to be left or center aligned. This means the actions have to be on the right, to make sure your eyes don't see them before the title.
@acrouthamel, if you'd like to land this on stable, then you'll need to re-base it on the stable branch. See https://community.kde.org/Infrastructure/Phabricator#Landing_on_the_.22Stable_branch.22
One more thing I just noticed, if you want bonus points:
Now that looks professional! Let's ship it!
Hmm, not sure I agree. The -symbolic icons exist specifically for this purpose: to always show a "symbolic" version. The non-symbolic version happens to look identical at a small size, but when scaled up,
I think there's one final bit of polish we can do here. In the "after" screenshots, most icons use the "symbolic" line-art stylr with Breeze, but a few show colored folders when run in HiDPI mode, which your screenshots depict. Perhaps we should take the opportunity to fix this, too. It's especially odd for the Trash icon.
Looks perfect now! Landing it on master.
Can this land?
In case anyone wants a more complex test file, I'm attaching one (written by me, MIT license):
Related bug: https://bugs.kde.org/show_bug.cgi?id=350688
Yes, these can't be Frameworks changes via KStandardAction because they're all Gwenview-specific. There is still one missing icon in context menus: Edit Tags. @acrouthamel, if you want to add that one too, that would be nice. tag-new seems like an appropriate icon.
Wonderful. Works great! Those missing icons always bugged me, too.
Wed, Feb 21
Thanks! I'll land this later tonight, if Alex doesn't beat me to it.
Since this is a small bugfix, I'd like to land it on the Stable branch (Plasma 5.12). However, I can't easily do this using arc because you did the work on master without a branch (in the future, see https://community.kde.org/Infrastructure/Phabricator#Workflow). Can you put it on a branch and re-base this patch on Plasma 5.12 branch? Here's how to do that:
Much better! Still needs a few things:
- Actual testing steps listed in the Test Plan section, not just a listing of your test config
- We can still remove more code. See D10711#210888. We can get rid of every usage of UseDynamicSaveButton and a checkbox in generalOptionsPage.h, and finally simplify KSMainWindow::saveButtonMode()
Thanks! Still gotta remove "BUG: 390748" from the title, though.
Also, please mention your testing process in the Test Plan section, and be sure to add the Spectacle group in the future.
Migrating my comment from D10718, since I saw that one before this one. We'll use your patch, since you got in first. :)
Thanks! Great second patch. There's even more code clean-up we can do here, though. For example:
Thanks for the patch! Instead of "Implementation of https://bugs.kde.org/show_bug.cgi?id=378463", could you change that to "BUG: 378463"? See https://community.kde.org/Infrastructure/Phabricator#Formatting_your_patch
Use of Latte Dock, maybe? No idea.
Thanks @rkflx, great catch. Can reproduce the issue, and your patch fixes it. Nicely done.
Works for me:
Tue, Feb 20
Thanks for the patch! Please remove "bug 390748" from the title and instead add "BUG: 390748" to its own line in the Summary section. See https://community.kde.org/Infrastructure/Phabricator#Formatting_your_patch