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

WIP: New clock plugin #408

Closed
wants to merge 10 commits into from
Closed

WIP: New clock plugin #408

wants to merge 10 commits into from

Conversation

tsimonq2
Copy link
Member

This solves issue 312 on the main tracker by merging the Clock and World Clock plugins.

I moved the World Clock plugin to the new location, worked on making sure everything says "Clock," and then I implemented the tooltip text. After that, I reworked the configuration UI a bit to make it look better.

tsimonq2 added 10 commits May 25, 2017 17:28
 - Remove the "General" menu because there was only one checkmark, and it was applicable in the new location anyways.
 - Change the formatting so that the advanced format checkbox and the customize button are on the same line, and it looks better this way.
 - General grammar and punctuation fixes.
@pepa65
Copy link

pepa65 commented May 27, 2017

I was eager to try out the result so far, so I started up a fresh LXQt in a VM, cloned your repo, installed cmake, ran cmake . and it came back with:

CMake Error at CMakeLists.txt:50 (find_package):
  By not providing "FindQt5Widgets.cmake" in CMAKE_MODULE_PATH this project
  has asked CMake to find a package configuration file provided by
  "Qt5Widgets", but CMake did not find one.

  Could not find a package configuration file provided by "Qt5Widgets" with
  any of the following names:

    Qt5WidgetsConfig.cmake
    qt5widgets-config.cmake

  Add the installation prefix of "Qt5Widgets" to CMAKE_PREFIX_PATH or set
  "Qt5Widgets_DIR" to a directory containing one of the above files.  If
  "Qt5Widgets" provides a separate development package or SDK, be sure it has
  been installed.


-- Configuring incomplete, errors occurred!
See also "/home/extix/git/lxqt-panel/CMakeFiles/CMakeOutput.log".
extix@extix:~/git/lxqt-panel$ less /home/extix/git/lxqt-panel/CMakeFiles/CMakeOutput.log

What else do I need to get this to build?

@yan12125
Copy link
Member

@pepa65: that message usually comes up if you don't install Qt5 dev libraries. https://github.com/lxde/lxqt/wiki/Building-from-source#qt should help

@pepa65
Copy link

pepa65 commented May 27, 2017

That helped somewhat, now I get:

CMake Error at CMakeLists.txt:57 (find_package):
  By not providing "Findlxqt.cmake" in CMAKE_MODULE_PATH this project has
  asked CMake to find a package configuration file provided by "lxqt", but
  CMake did not find one.

  Could not find a package configuration file provided by "lxqt" with any of
  the following names:

    lxqtConfig.cmake
    lxqt-config.cmake

  Add the installation prefix of "lxqt" to CMAKE_PREFIX_PATH or set
  "lxqt_DIR" to a directory containing one of the above files.  If "lxqt"
  provides a separate development package or SDK, be sure it has been
  installed.


-- Configuring incomplete, errors occurred!
See also "/home/extix/git/lxqt-panel/CMakeFiles/CMakeOutput.log".

@yan12125
Copy link
Member

liblxqt and more packages are necessary. lxqt-panel requires almost all other components in lxqt to be installed first. See https://github.com/lxde/lxqt/wiki/Building-from-source#generalbuild-order for details.

@agaida
Copy link
Member

agaida commented May 27, 2017

on debian/ubuntu systems apt build-dep lxqt-panel but i would suggest to learn some system maintenance basics first. Never ever compile and install things from source unless one is damn sure about what to do. But hey - forced new installs are funny too.

@pepa65
Copy link

pepa65 commented May 27, 2017

OK, thanks, after installing liblxqt* and then libsysstat-qt5-0-dev cmake finishes and make seems to build and install!

But after adding worldclock to the panel, and adding timezones, it doesn't display anything different. Left-click just shows the calendar, nothing on hover...

@agaida
Copy link
Member

agaida commented May 27, 2017

two possible reasons - a) there are not so much changes yet b) one run the wrong binary

@agaida
Copy link
Member

agaida commented May 27, 2017

@tsimonq2 - before i forget about - one should also take care of the translations - have a look at lxqt-l10n

Edit: possible changes should be handled in a different PR
Edit 2: Applied the PR local, nice work so far

@palinek
Copy link
Contributor

palinek commented May 29, 2017

How about existing user configurations?

  • if the user used a clock plugin, after update she/he will get the new (world)clock with default configuration (as the two plugins doesn't share any configuration parameters)
  • if the user used a worldclock plugin, after update she/he will be w/o any clock in panel and in the configuration dialog there will be an "unknown" worldclock

Is this acceptable?

@pepa65
Copy link

pepa65 commented May 29, 2017

@palinek Excellent points. Indeed, the worldclock plugin/package should keep its name because of config files. When all is done, the default setup would use the worldclock plugin.

@tsimonq2
Copy link
Member Author

@agaida: ack, I'm thinking this is something we can address after this is merged.

I agree with @palinek... the thought crossed my mind but I spaced it before I sent my PR. I think we could do one of two things:

  1. Prompt the user if they have the regular clock plugin and/or the worldclock plugin enabled on their panel to do one of a couple things (TBD) to ease migration.
  2. Figure out a way to convert existing clock plugin configurations to the new clock one, JDFI and just convert the plugins.

I really disagree with @pepa65 irt just renaming to worldclock because the wording on "clock" is a bit better than "worldclock," unless everyone thinks it would be easier to either:

  1. Only keep worldclock and convert all the clock configurations to worldclock. or
  2. Rename this new clock plugin to something else so we can migrate both plugins to the new plugin.

Thoughts?

@tsujan
Copy link
Member

tsujan commented May 29, 2017

Keeping worldclock seems fine to me not just because I use it but also because it's very customizable.

@tsimonq2
Copy link
Member Author

@tsujan: The base for this new clock plugin is literally just worldclock with a few improvements and the naming changed to "clock". Don't worry, I agree with you. :)

@tsujan
Copy link
Member

tsujan commented May 29, 2017

The base for this new clock plugin is literally just worldclock with a few improvements

Great! Personally, I don't care about losing worldclock after update, provided that the new clock has all the features of worldclock and can be added to the panel without problem. Some users might get unhappy seeing their clock is missing after update but that isn't a big deal.

@tsimonq2 tsimonq2 requested review from palinek and agaida May 31, 2017 03:08
@palinek
Copy link
Contributor

palinek commented Jun 6, 2017

IMO doing the merge this way is not good, because it will cause confusion for users and for bug reporting (mixing new clock with old clock and old worldclock).
I would suggest to make the here-included improvements to the worldclock w/o renaming. Then:

  • either drop the clock and make some "conversion logic" like e.g. here
  • or mark the clock as "deprecated" and showing the "...clock is deprecated and will be dropped in future release..." message after panel start (of course with option "don't show this message again")

@tsimonq2 what do you think?

@agaida
Copy link
Member

agaida commented Jun 6, 2017

@palinek - this would work too. If we decide to go that way i would only add the deprecation message and thats all. The clock settings are three clicks - so it don't make sense to do more right now.

@agaida agaida changed the title New clock plugin WIP: New clock plugin Sep 16, 2017
@tsujan
Copy link
Member

tsujan commented Sep 18, 2017

Please just remove Date & Time completely! World Clock is good and enough. IMHO, this isn't worth a long discussion.

@palinek
Copy link
Contributor

palinek commented Sep 20, 2017

Please, consider the #426

@palinek
Copy link
Contributor

palinek commented Sep 20, 2017

As the #426 was merged already, closing this.

@tsimonq2 please, make a new PR with any outstanding changes from this one.

@palinek palinek closed this Sep 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants